feat: 技能支持多文件导入，新增/编辑改为单一 SKILL.md 编辑器

[xianxh]

Summary

• 技能页头部「新建」「导入」两个按钮合并为单一「新建技能」入口；弹框内用分段切换区分「新建」（粘贴 SKILL.md，走 create）与「导入文件 / 文件夹」（走 import）。粘贴即新建，只有文件/文件夹才算导入。
• 新增 / 编辑 / 复制统一为单一 SKILL.md markdown 编辑器（前端 utils/skillmd.ts 解析/序列化，镜像 daemon 格式）。
• daemon 支持从本地文件夹 / SKILL.md 路径导入多文件技能：createSkill 以 sourceDir 原样拷贝整棵目录，sync 镜像整个内容目录（含 references/scripts 同级文件），目标目录每次重建避免残留旧文件。
• desktop 新增 showSkillFilePicker（.md 文件选择器）；浏览器 / NAS 无原生选择器时降级为手填容器路径。

Test plan

• pnpm --filter @molio/web typecheck 通过
• pnpm desktop:run 重新打包成功（win-unpacked）
• 手动验证：技能页只有一个「新建技能」按钮；弹框「新建 / 导入文件 / 文件夹」切换正常
• skills E2E：cd apps/web && npx playwright test e2e/skills.spec.ts（需先 pnpm dev）
• daemon 单测：pnpm --filter @molio/daemon test

[github-actions]

🔍 OpenCodeReview found 26 issue(s) in this PR.

• ✅ 26 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

scratchDir returns one fixed directory (~/.molio/scratch) that prefill.ts uses as the cwd for throwaway Claude runs (see ensureScratchCwd / createRun({ cwd })). Two concurrent "Save as skill" prefill requests will therefore share the exact same working directory. If the spawned process writes any per-run state into its cwd (session/cache/temp files with predictable names), concurrent runs can clobber or read each other's files, producing corrupted prefill output or intermittent failures. Consider giving each run a unique subdir (e.g. path.join(scratchDir(opts), randomUUID())) so concurrent runs are isolated, and clean it up on settle.

[github-actions]

Bug (medium): When a core skill already exists, refreshMeta only updates name/description but never refreshes instructions. Since core skills are explicitly "hidden + always-on + not configurable" — users cannot edit their instructions — the hardcoded CORE_SKILLS_SEEDS prompts are the sole source of truth. Yet they are frozen at first-seed time: once the row exists, any prompt improvement or bug-fix in a future app version will never reach existing installations.

This is inconsistent with the stated design: name/description are refreshed every startup (so the UI stays in sync), but the actual instructions driving agent behavior are silently locked to whatever was seeded on first run.

Consider also refreshing instructions (and rewriting the on-disk SKILL.md via writeSkillMd + re-syncing vaults) for core: true skills during seeding, since there is no user-editable state to preserve for them.

[github-actions]

Bug (missing error handling at system boundary): Unlike initSkillLibrary (internal try/catch) and reconcileAllVaults (per-vault try/catch inside reconcileVault), cleanupLegacyGlobalSync has no error protection — neither internally nor at this call site. Internally it calls reconcileSync([], …) whose orphan-cleanup loop does fs.rmSync(…) on each molio--* dir without a try/catch. If any rmSync throws (e.g. EACCES on a root-owned dir, or a file locked on Windows), the exception propagates here and crashes the daemon before startServer() is ever reached.

This contradicts the comment ("idempotent; safe to run every startup") and the best-effort design used everywhere else in this startup sequence. Wrap the call (or add a try/catch inside cleanupLegacyGlobalSync) so a cleanup failure degrades to a warning rather than preventing startup.

Suggestion:

Suggested change

	cleanupLegacyGlobalSync();
	try {
	cleanupLegacyGlobalSync();
	} catch (err) {
	console.warn('[skills] Legacy global sync cleanup failed (non-fatal):', err instanceof Error ? err.message : err);
	}

[github-actions]

Bug (missing error handling at fs boundary): fs.readFileSync can throw raw system errors (EISDIR if SKILL.md is actually a directory, EACCES for permission issues, or ENOENT from a TOCTOU race between existsSync in resolveSource and this read). These propagate as unhandled system errors through the route's catch-all, returning a generic 500 instead of a meaningful SkillImportError.

Notably, resolveSource only checks fs.existsSync(skillMd) for directories — it does not verify that SKILL.md is a file (not a subdirectory). A directory named SKILL.md would pass the check and crash here with EISDIR.

Consider wrapping the read (and optionally the resolveSource check) in a try-catch that maps fs errors to SkillImportError:

Suggestion:

Suggested change

	const src = resolveSource(folderPath.trim());
	const raw = fs.readFileSync(src.skillMd, 'utf8');
	const src = resolveSource(folderPath.trim());
	let raw: string;
	try {
	raw = fs.readFileSync(src.skillMd, 'utf8');
	} catch (err: any) {
	throw new SkillImportError('NOT_FOUND', `无法读取 ${src.skillMd}：${err?.message ?? err}`);
	}

[github-actions]

Both reconcileSync and reconcileBundledSync are wrapped in a single try/catch. If reconcileSync throws (its orphan-cleanup section — fs.readdirSync / fs.rmSync — is NOT individually try/caught, unlike the per-skill sync loop), the entirely independent bundled sync is silently skipped for this vault, and the generic warning message won't reveal which half failed.

Since the two operations target different directories (molio--* single-file dirs vs. plain-name multi-file dirs), they are fully independent. Consider giving each its own try/catch so a failure in one doesn't prevent the other from running.

Suggestion:

Suggested change

	const singleFileIds = effective.filter((s) => s.kind !== 'bundled').map((s) => s.id);
	reconcileSync(singleFileIds, { ...opts, claudeHome: path.join(vault.path, '.claude') });
	const singleFileIds = effective.filter((s) => s.kind !== 'bundled').map((s) => s.id);
	try {
	reconcileSync(singleFileIds, { ...opts, claudeHome: path.join(vault.path, '.claude') });
	} catch (err) {
	console.warn(`[skills] Failed to sync library/core skills into vault "${vault.name}":`, err instanceof Error ? err.message : err);
	}

[github-actions]

Same two-phase pattern as openEdit, with an additional failure mode: show is derived from modal !== null, so if the user closes the empty 'create' modal before getSkill resolves, this second setModal re-opens the closed modal with the duplicated content. It also clobbers anything the user typed into the empty create editor while the fetch was in flight.

Simplest robust fix: fetch first, then open the modal once with initialMarkdown already populated (and only if the flow wasn't cancelled).

[github-actions]

SkillResponse does not match the actual GET /api/skills/:id response. The daemon returns { skill, instructions: readInstructions(skill.id) } and the web client types it as { skill: SkillManifestEntry; instructions: string } (this is how the edit form populates the SKILL.md editor). Declaring only { skill } here makes the contract incomplete/misleading for the very endpoint that feeds the edit form. Also note that SkillResponse, SkillListResponse, PrefillResponse, and VaultSkillListResponse are not referenced by either the daemon or the web client (the client builds inline types), so these wrappers risk drifting from reality. Suggest adding instructions (and any other fields the endpoint returns) or aligning/removing the unused wrappers so the contract reflects the real payloads.

Suggestion:

Suggested change

	export interface SkillResponse {
	skill: SkillManifestEntry;
	}
	export interface SkillResponse {
	skill: SkillManifestEntry;
	/** SKILL.md body for the edit form (returned by GET /api/skills/:id). */
	instructions: string;
	}

[github-actions]

The comment says exactly one of raw/folderPath must be provided, but the type makes both optional and leaves the constraint to runtime enforcement in the daemon route. Consider encoding the mutual exclusivity at the type level (an XOR/discriminated union) so clients that omit both or pass both fail at compile time instead of only at request time.

Suggestion:

Suggested change

	export interface ImportSkillRequest {
	raw?: string;
	folderPath?: string;
	}
	export type ImportSkillRequest =
	| { raw: string; folderPath?: never }
	| { raw?: never; folderPath: string };

[github-actions]

VaultSkillEntry duplicates most metadata fields of SkillManifestEntry (id, name, description, builtIn, kind, createdAt, updatedAt) and only differs in the enablement flags. Extracting a shared base (e.g. SkillBase) and extending it for both would reduce the risk of one representation being updated without the other.

[github-actions]

The \s* in this regex matches newlines (the m flag only affects ^/$, not \s). When a frontmatter value is empty or whitespace-only, the greedy \s* consumes the trailing line break and (.*) then captures the NEXT line. For a block like name:\ndescription: foo, frontmatterField(content, 'name') returns 'description: foo' instead of ''.

This is reachable on the save path: SkillFormModal.validateMarkdown relies on if (!parsed.name.trim()) to reject an empty name, but the mis-captured non-empty value slips past that guard and a skill gets created/updated with a corrupted name.

Restrict the separator to horizontal whitespace so it can never cross a line boundary. (The daemon mirror in apps/daemon/src/core/skills/skillmd.ts has the same regex and should be fixed consistently.)

Suggestion:

Suggested change

	const re = new RegExp(`^${key}:\\s*(.*)$`, 'm');
	const re = new RegExp(`^${key}:[ \\t]*(.*)$`, 'm');

[github-actions]

🔍 OpenCodeReview found 28 issue(s) in this PR.

• ✅ 28 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

Path traversal risk: id is interpolated directly into filesystem paths without validation. Currently all callers use UUIDs or DB-validated IDs, but this module is the shared path boundary for the skill system and a malicious/corrupted id (e.g. ../../etc) would resolve outside the intended directories. Consider adding a guard here (defense-in-depth) so that any future caller is protected:

const SAFE_ID = /^[a-zA-Z0-9_-]+$/;
function assertSafeId(id: string): void {
  if (!SAFE_ID.test(id)) throw new Error(`Invalid skill id: ${id}`);
}

Call it at the top of skillContentDir and molioSkillDir.

[github-actions]

Bug: existsSync returns true for directories as well as files. If a user's skill folder contains a directory named SKILL.md (or a broken symlink), this check passes, but readFileSync(src.skillMd, 'utf8') in importFromFolder will then throw EISDIR (or ELOOP/EACCES). That raw Node error propagates as an unhelpful HTTP 500 instead of a clear SkillImportError.

Consider validating that SKILL.md is actually a regular file, and wrapping the subsequent readFileSync in a try/catch that converts fs errors into SkillImportError:

Suggestion:

Suggested change

	const skillMd = path.join(input, 'SKILL.md');
	if (!fs.existsSync(skillMd)) {
	throw new SkillImportError('NOT_FOUND', `该文件夹根目录没有 SKILL.md：${input}`);
	}
	const skillMd = path.join(input, 'SKILL.md');
	let skillMdStat: fs.Stats | null = null;
	try { skillMdStat = fs.statSync(skillMd); } catch { /* not found */ }
	if (!skillMdStat?.isFile()) {
	throw new SkillImportError('NOT_FOUND', `该文件夹根目录没有 SKILL.md：${input}`);
	}

[github-actions]

Missing error handling at fs boundary: readFileSync can throw for reasons resolveSource doesn't guard against — permission errors (EACCES), a TOCTOU race (file removed between statSync and read), encoding errors, or the file being replaced by a directory. These surface as raw Node errors → HTTP 500 rather than a user-actionable SkillImportError. Consider wrapping the read in try/catch and rethrowing as SkillImportError('BAD_REQUEST', ...) so the route returns a meaningful 4xx.

Suggestion:

Suggested change

	const src = resolveSource(folderPath.trim());
	const raw = fs.readFileSync(src.skillMd, 'utf8');
	const src = resolveSource(folderPath.trim());
	let raw: string;
	try {
	raw = fs.readFileSync(src.skillMd, 'utf8');
	} catch (err) {
	throw new SkillImportError('BAD_REQUEST', `无法读取 SKILL.md：${err instanceof Error ? err.message : err}`);
	}

[github-actions]

The orphan-cleanup fs.rmSync here is not wrapped in try/catch, unlike the syncSkill loop just above which guards each id individually. A single filesystem error on one orphaned entry (e.g. EACCES on a root-owned mounted docs dir, or EBUSY) will throw out of the loop and abort cleanup of all remaining molio--* orphans in this vault, leaving partial reconciliation state. This also contradicts the module's documented best-effort contract (vault-config.ts: "an EACCES there must degrade to a warning, never abort"). Wrap the removal per-entry so one bad directory doesn't skip the rest, mirroring the error handling used for syncSkill.

Suggestion:

Suggested change

	const id = entry.name.slice(MOLIO_PREFIX.length);
	if (!enabledSet.has(id)) {
	fs.rmSync(path.join(dir, entry.name), { recursive: true, force: true });
	}
	const id = entry.name.slice(MOLIO_PREFIX.length);
	if (!enabledSet.has(id)) {
	try {
	fs.rmSync(path.join(dir, entry.name), { recursive: true, force: true });
	} catch (err) {
	console.error(`[skills] Failed to remove orphaned skill dir "${entry.name}":`, err instanceof Error ? err.message : err);
	}
	}

[github-actions]

The try/catch wraps both DB reads (getEffectiveSkills → listSkills, getVaultSkillOverrides) and filesystem writes, but the log message always says "likely a write-permission problem on the directory." A genuine DB failure (database locked, corruption, schema mismatch) would be swallowed and misdiagnosed as a permissions issue, potentially leaving all vaults persistently unsynced with no actionable log to guide debugging.

Consider separating the DB-read phase from the FS-write phase so that DB errors propagate (or at least get a distinct, accurate log message), while only FS errors get the best-effort EACCES treatment the doc comment describes.

Suggestion:

Suggested change

	try {
	const effective = getEffectiveSkills(db, vault.id);
	// library + core → molio-- single-file sync (orphan cleanup included).
	const singleFileIds = effective.filter((s) => s.kind !== 'bundled').map((s) => s.id);
	reconcileSync(singleFileIds, { ...opts, claudeHome: path.join(vault.path, '.claude') });
	// bundled → whole-dir sync. Managed = every bundled row the DB knows about
	// (so a toggled-off one gets removed); effective = the subset that's on.
	const allSkills = listSkills(db);
	const managedBundled = new Set(allSkills.filter((s) => s.kind === 'bundled').map((s) => s.id));
	const effectiveBundled = new Set(effective.filter((s) => s.kind === 'bundled').map((s) => s.id));
	reconcileBundledSync(effectiveBundled, managedBundled, vault.path);
	} catch (err) {
	console.warn(
	`[skills] Failed to reconcile skills into vault "${vault.name}" (${vault.path}) — ` +
	`likely a write-permission problem on the directory. The vault is still usable. Cause:`,
	err instanceof Error ? err.message : err,
	);
	}
	let effective: ReturnType<typeof getEffectiveSkills>;
	let allSkills: ReturnType<typeof listSkills>;
	try {
	effective = getEffectiveSkills(db, vault.id);
	allSkills = listSkills(db);
	} catch (err) {
	console.error(
	`[skills] DB error while computing effective skills for vault "${vault.name}" (${vault.id}):`,
	err instanceof Error ? err.message : err,
	);
	return;
	}
	try {
	// library + core → molio-- single-file sync (orphan cleanup included).
	const singleFileIds = effective.filter((s) => s.kind !== 'bundled').map((s) => s.id);
	reconcileSync(singleFileIds, { ...opts, claudeHome: path.join(vault.path, '.claude') });
	// bundled → whole-dir sync.
	const managedBundled = new Set(allSkills.filter((s) => s.kind === 'bundled').map((s) => s.id));
	const effectiveBundled = new Set(effective.filter((s) => s.kind === 'bundled').map((s) => s.id));
	reconcileBundledSync(effectiveBundled, managedBundled, vault.path);
	} catch (err) {
	console.warn(
	`[skills] Failed to reconcile skills into vault "${vault.name}" (${vault.path}) — ` +
	`likely a write-permission problem on the directory. The vault is still usable. Cause:`,
	err instanceof Error ? err.message : err,
	);
	}

[github-actions]

toggle re-throws on failure (the hook catches the API error, rolls back the optimistic flip, then does throw err). Calling it via void toggle(...) discards the returned promise without attaching a .catch() — void only evaluates the expression, it does not handle rejections. So when a toggle fails (network/server error), this produces an unhandled promise rejection in the console, and the switch silently snaps back with no feedback to the user. Catch the rejection (and ideally surface the error) at the call site.

Suggestion:

Suggested change

	onToggle={(enabled) => void toggle(skill.id, enabled)}
	onToggle={(enabled) => {
	void toggle(skill.id, enabled).catch((err) => {
	// toggle already rolls back the optimistic flip; surface/log the failure
	console.error('Failed to toggle vault skill', err);
	});
	}}

[github-actions]

Race condition: this hook is mounted persistently (VaultSkillsModal is always rendered and calls the hook before if (!show) return null), and vaultId = kb.activeVault?.id changes on every vault switch — triggering refresh in the background even when the modal is closed. Concurrent fetches are not guarded by a request token or AbortController, so if the user switches vault A → B quickly, a slow response for A can resolve after B's and call setSkills with vault A's entries (including A's vaultEnabled values) while the current vault is B. That older request's finally can also set loading=false while B's request is still pending. Capture a monotonically increasing request id (or the current vaultId in a ref) and bail out when the response is stale.

Suggestion:

Suggested change

	const data = await api.listVaultSkills(vaultId);
	setSkills(data);
	const reqId = ++reqCounter.current;
	const data = await api.listVaultSkills(vaultId);
	if (reqId !== reqCounter.current) return; // stale response for a previous vault
	setSkills(data);

[github-actions]

Stale-response merge: vaultId is captured at call time. If the active vault changes (or a refresh replaces the list) before this PATCH resolves, upsert(skill) still merges the previous vault's entry into the current list. Because skill ids are shared across vaults, this overwrites the current vault's displayed vaultEnabled with the old vault's value (or appends a stale entry if missing), so the UI shows the wrong toggle state and the next toggle is computed from it. Verify the captured vaultId still matches the active vault before upserting (e.g. via a vaultIdRef kept in sync each render), or skip the upsert and rely on the next refresh.

Suggestion:

Suggested change

	const skill = await api.setVaultSkillEnabled(vaultId, skillId, enabled);
	upsert(skill);
	const skill = await api.setVaultSkillEnabled(vaultId, skillId, enabled);
	if (vaultIdRef.current !== vaultId) return; // vault switched mid-flight; drop stale entry
	upsert(skill);

[github-actions]

skill is listed as a dependency but is never read inside the effect body. Since this effect resets ALL form fields whenever any dependency changes while show is true, an identity change of the unused skill prop would silently discard everything the user has typed. It happens to be dormant today only because SkillsPanel/App.tsx store skill/prefillData in state (stable references) — but the moment a caller derives skill inline (e.g. skill={skills.find(...)} creating a fresh object per render), the form will reset on every parent re-render. Remove skill from the dependency array since it isn't used.

Suggestion:

Suggested change

	}, [show, mode, skill, prefillData, initialMarkdown]);
	}, [show, mode, prefillData, initialMarkdown]);

[github-actions]

Switching between the paste and import sources only calls setSource(...); it never clears fieldError. So a validation error raised under one source (e.g. "folder required" from the import source) stays visible after switching to the other source, displayed above unrelated form fields. Clear the error when the source changes — apply the same to the paste button's handler.

Suggestion:

Suggested change

	onClick={() => setSource('import')}
	onClick={() => { setSource('import'); setFieldError(null); }}

[xianxh]

/open-code-review

[github-actions]

🔍 OpenCodeReview found 70 issue(s) in this PR.

• ✅ 69 posted as inline comment(s)
• 📝 1 posted as summary

---

📄 packages/contracts/src/skillmd.ts

frontmatterField builds its matcher with new RegExp(^${key}\s*:\s*(.*)$) without escaping the key. Current callers only pass the compile-time constants 'name'/'description', so there is no issue today, but a future caller passing any user-influenced input would inject regex syntax and break/misparse fields. Escape the key, e.g. key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), to make the function safe by construction.

[github-actions]

The id parameter is interpolated into a filesystem path via path.join() with no validation or normalization. path.join() resolves .. segments and keeps / separators, so an id like ../../foo or a/b would escape ~/.molio/skills/the molio-- namespace or create unintended nested dirs — violating the isolation this module's docstring promises. Today all callers happen to pass DB-sourced ids (randomUUID or hardcoded slugs), so this isn't reachable through the routes, but this helper is the single security boundary for the skill library, and one future caller passing user input (route param, SKILL.md frontmatter, imported filename) would silently turn into arbitrary file read/write/delete. Recommend validating the id is a single safe path segment inside these helpers (e.g. reject empty, ./.., /, \, and path.basename mismatches) so the invariant is enforced at the boundary regardless of caller.

Suggestion:

Suggested change

	export function skillContentDir(id: string, opts?: SkillPathsOpts): string {
	return path.join(skillsDir(opts), id);
	}
	function assertSafeSkillId(id: string): void {
	if (!id || id.length > 128 || id !== path.basename(id) || id === '.' || id === '..') {
	throw new Error(`Invalid skill id: ${id}`);
	}
	}
	export function skillContentDir(id: string, opts?: SkillPathsOpts): string {
	assertSafeSkillId(id);
	return path.join(skillsDir(opts), id);
	}

[github-actions]

The summary is suppressed unless something was actually removed. If the sweep fails on every expired directory (e.g., EACCES, or a persistent lock on Windows), failed grows while removed stays 0 and no diagnostic is emitted at all — the disk fills up silently. This helper is now shared by the production startup path (pruneRunLogsAsync), whose result is discarded by the caller in index.ts, so per-entry failures are completely invisible. Log the summary whenever failed > 0 as well.

Suggestion:

Suggested change

	function logPruneSummary(result: PruneRunLogsResult, dir: string): void {
	if (result.removed > 0) {
	function logPruneSummary(result: PruneRunLogsResult, dir: string): void {
	if (result.removed > 0 || result.failed > 0) {

[github-actions]

This catch swallows every readdirSync failure, not just ENOENT. A permission error (EACCES) on ~/.molio/runs is silently treated as "nothing to do" and, since the caller discards the result, the failure is completely invisible. Distinguish ENOENT from other errors and log the latter so an unwritable/pruned-by-other-process directory is diagnosable.

Suggestion:

Suggested change

	let entries: string[];
	try {
	entries = fs.readdirSync(dir);
	} catch {
	// Directory doesn't exist yet (fresh install) — nothing to do.
	return result;
	}
	let entries: string[];
	try {
	entries = fs.readdirSync(dir);
	} catch (err) {
	if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
	console.warn(`[runs-log-prune] cannot read ${dir}:`, err);
	}
	return result;
	}

[github-actions]

The lone-file import path skips the size guard entirely: assertFolderWithinLimits is only invoked for dir sources, and resolveSource's file branch accepts ANY file (no .md extension check). Pointing the import at a multi-GB arbitrary file makes fs.readFileSync buffer the whole file into memory, risking daemon OOM. MAX_IMPORT_BYTES is defined but never enforced on this path. Apply the size limit before reading (and ideally restrict the file branch to .md files, matching the doc comment).

Suggestion:

Suggested change

	if (src.type === 'dir') assertFolderWithinLimits(src.dir);
	const raw = fs.readFileSync(src.skillMd, 'utf8');
	if (src.type === 'dir') {
	assertFolderWithinLimits(src.dir);
	} else {
	const size = fs.statSync(src.skillMd).size;
	if (size > MAX_IMPORT_BYTES) {
	throw new SkillImportError(
	'BAD_REQUEST',
	`导入文件大小超过上限（最大 ${Math.round(MAX_IMPORT_BYTES / 1024 / 1024)} MB）`,
	);
	}
	}
	const raw = fs.readFileSync(src.skillMd, 'utf8');

[github-actions]

The limit walk and the actual copy disagree on symlinks: statSync here follows symlinks, but copyDirSync treats symlinks as regular entries (Dirent.isDirectory() is false for a symlink) and copies them with fs.copyFileSync. A symlink pointing at a directory passes the walk (counted as a single small 'file') but makes copyFileSync throw EISDIR mid-copy — the import dies with an unhandled fs error after partially copying files into ~/.molio/skills/<id>/, orphaning that content dir. Broken symlinks are likewise skipped by this walk (continue) yet throw ENOENT in the copy. Handle symlinks explicitly (lstat + skip/resolve) and clean up the partial destination on copy failure so imports fail cleanly instead of leaking partial content.

[github-actions]

firstHeadingText toggles inFence on any line starting with or ~~~ without tracking fence character or length. With a mismatched pair (e.g. opened with but closed with ~~~~, or a ~~~ strikethrough line mis-detected as a fence opener), inFence inverts and all subsequent real headings are skipped during name fallback, silently worsening the derived name. Track the fence marker (e.g. store the opening string and only close on a matching/equally-long delimiter) instead of a bare boolean toggle.

[github-actions]

The collapsed-field/block-scalar splitting logic is duplicated between expandCollapsedFields (fenced path) and the inline loop in splitFrontmatter (unfenced path). Both must stay in sync; a future fix to one path (e.g. constraining COLLAPSED_FIELD to avoid value corruption) could leave the other inconsistent, causing divergent parse results between fenced and unfenced pastes. Consider extracting a single shared line-splitting helper used by both paths.

[github-actions]

readBlockScalar ignores YAML chomping indicators (|+ should keep trailing newlines, >- should strip them) and collapses multiple blank lines to a single \n via \n{2,} → \n. For folded > scalars this also loses the distinction between a single newline (→ space) and a paragraph break. Round-tripped multi-paragraph descriptions are therefore not byte-identical to the original — minor for a tolerant parser, but worth noting if the importer/editor ever relies on preserving description formatting. If exact fidelity isn't required, at least document the lossy behavior.

[github-actions]

firstHeadingText toggles inFence on any line starting with ``` or ~~~ without tracking fence character or length. With a mismatched pair (e.g. opened with ``` but closed with ~~~~, or a ~~~ strikethrough line mis-detected as a fence opener), inFence inverts and all subsequent real headings are skipped during name fallback, silently worsening the derived name. Track the fence marker (e.g. store the opening string and only close on a matching/equally-long delimiter) instead of a bare boolean toggle.

[github-actions]

The collapsed-field/block-scalar splitting logic is duplicated between expandCollapsedFields (fenced path) and the inline loop in splitFrontmatter (unfenced path). Both must stay in sync; a future fix to one path (e.g. constraining COLLAPSED_FIELD to avoid value corruption) could leave the other inconsistent, causing divergent parse results between fenced and unfenced pastes. Consider extracting a single shared line-splitting helper used by both paths.

[github-actions]

The id parameter is interpolated into a filesystem path via path.join() with no validation or normalization. path.join() resolves .. segments and keeps / separators, so an id like ../../foo or a/b would escape ~/.molio/skills/the molio-- namespace or create unintended nested dirs — violating the isolation this module's docstring promises. Today all callers happen to pass DB-sourced ids (randomUUID or hardcoded slugs), so this isn't reachable through the routes, but this helper is the single security boundary for the skill library, and one future caller passing user input (route param, SKILL.md frontmatter, imported filename) would silently turn into arbitrary file read/write/delete. Recommend validating the id is a single safe path segment inside these helpers (e.g. reject empty, ./.., /, \, and path.basename mismatches) so the invariant is enforced at the boundary regardless of caller.

Suggestion:

Suggested change

	export function skillContentDir(id: string, opts?: SkillPathsOpts): string {
	return path.join(skillsDir(opts), id);
	}
	function assertSafeSkillId(id: string): void {
	if (!id || id.length > 128 || id !== path.basename(id) || id === '.' || id === '..') {
	throw new Error(`Invalid skill id: ${id}`);
	}
	}
	export function skillContentDir(id: string, opts?: SkillPathsOpts): string {
	assertSafeSkillId(id);
	return path.join(skillsDir(opts), id);
	}

[github-actions]

The summary is suppressed unless something was actually removed. If the sweep fails on every expired directory (e.g., EACCES, or a persistent lock on Windows), failed grows while removed stays 0 and no diagnostic is emitted at all — the disk fills up silently. This helper is now shared by the production startup path (pruneRunLogsAsync), whose result is discarded by the caller in index.ts, so per-entry failures are completely invisible. Log the summary whenever failed > 0 as well.

Suggestion:

Suggested change

	function logPruneSummary(result: PruneRunLogsResult, dir: string): void {
	if (result.removed > 0) {
	function logPruneSummary(result: PruneRunLogsResult, dir: string): void {
	if (result.removed > 0 || result.failed > 0) {

[github-actions]

This catch swallows every readdirSync failure, not just ENOENT. A permission error (EACCES) on ~/.molio/runs is silently treated as "nothing to do" and, since the caller discards the result, the failure is completely invisible. Distinguish ENOENT from other errors and log the latter so an unwritable/pruned-by-other-process directory is diagnosable.

Suggestion:

Suggested change

	let entries: string[];
	try {
	entries = fs.readdirSync(dir);
	} catch {
	// Directory doesn't exist yet (fresh install) — nothing to do.
	return result;
	}
	let entries: string[];
	try {
	entries = fs.readdirSync(dir);
	} catch (err) {
	if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
	console.warn(`[runs-log-prune] cannot read ${dir}:`, err);
	}
	return result;
	}

[github-actions]

The lone-file import path skips the size guard entirely: assertFolderWithinLimits is only invoked for dir sources, and resolveSource's file branch accepts ANY file (no .md extension check). Pointing the import at a multi-GB arbitrary file makes fs.readFileSync buffer the whole file into memory, risking daemon OOM. MAX_IMPORT_BYTES is defined but never enforced on this path. Apply the size limit before reading (and ideally restrict the file branch to .md files, matching the doc comment).

Suggestion:

Suggested change

	if (src.type === 'dir') assertFolderWithinLimits(src.dir);
	const raw = fs.readFileSync(src.skillMd, 'utf8');
	if (src.type === 'dir') {
	assertFolderWithinLimits(src.dir);
	} else {
	const size = fs.statSync(src.skillMd).size;
	if (size > MAX_IMPORT_BYTES) {
	throw new SkillImportError(
	'BAD_REQUEST',
	`导入文件大小超过上限（最大 ${Math.round(MAX_IMPORT_BYTES / 1024 / 1024)} MB）`,
	);
	}
	}
	const raw = fs.readFileSync(src.skillMd, 'utf8');

[github-actions]

The limit walk and the actual copy disagree on symlinks: statSync here follows symlinks, but copyDirSync treats symlinks as regular entries (Dirent.isDirectory() is false for a symlink) and copies them with fs.copyFileSync. A symlink pointing at a directory passes the walk (counted as a single small 'file') but makes copyFileSync throw EISDIR mid-copy — the import dies with an unhandled fs error after partially copying files into ~/.molio/skills/<id>/, orphaning that content dir. Broken symlinks are likewise skipped by this walk (continue) yet throw ENOENT in the copy. Handle symlinks explicitly (lstat + skip/resolve) and clean up the partial destination on copy failure so imports fail cleanly instead of leaking partial content.

[github-actions]

The "atomic swap" claim only holds for the rename step, not the whole sequence. fs.rmSync(destDir) followed by fs.renameSync(tmp, destDir) leaves a window where concurrent readers (agent CLI scanning .claude/skills/) see ENOENT for the dest dir. Worse: if renameSync fails (EPERM on an ownership-changed NAS mount, AV lock on Windows), destDir has already been deleted and the catch only cleans up tmp — the skill dir is silently lost for that vault until the next reconcile. This contradicts the best-effort/EACCES-degrades-to-warning policy in vault-config.ts. Use a two-phase swap with a backup so a failed rename restores the previous copy instead of deleting it.

Suggestion:

Suggested change

	copyDirSync(srcDir, tmp);
	fs.rmSync(destDir, { recursive: true, force: true });
	fs.renameSync(tmp, destDir);
	return true;
	copyDirSync(srcDir, tmp);
	const backup = `${destDir}.bak-${Date.now()}-${Math.random().toString(36).slice(2)}`;
	if (fs.existsSync(destDir)) fs.renameSync(destDir, backup);
	try {
	fs.renameSync(tmp, destDir);
	} catch (err) {
	if (fs.existsSync(backup)) fs.renameSync(backup, destDir); // restore previous copy
	throw err;
	}
	fs.rmSync(backup, { recursive: true, force: true });
	return true;

[github-actions]

hashDir skips symlinks (neither isDirectory() nor isFile()), but copyDirSync copies them as regular files via copyFileSync, which follows the link. Consequences: (1) any source tree containing a symlink never hash-matches the copied dest, so that skill is rebuilt on every daemon start — defeating the short-circuit this module exists for, on exactly the NAS path it targets; (2) a symlink pointing at a directory makes copyFileSync throw EISDIR, crashing the whole mirror (the "never a wrong result" comment is not true for this case); (3) a symlink pointing outside the source copies external file content into the vault. Make symlink handling consistent in both functions (skip in both, or hash and copy them uniformly).

Suggestion:

Suggested change

	} else {
	fs.copyFileSync(srcPath, destPath);
	}
	if (entry.isDirectory()) {
	copyDirSync(srcPath, destPath);
	} else if (entry.isSymbolicLink()) {
	// Keep consistent with hashDir (which ignores symlinks): skip rather than
	// follow, so the dest hash can match and copyFileSync can't crash on a
	// link-to-directory or leak external file content into the vault.
	} else {
	fs.copyFileSync(srcPath, destPath);
	}

[github-actions]

hashDir reads each file fully into memory via fs.readFileSync with no per-file size guard. Although the importer caps a folder at 100MB/1000 files, a single file can still be ~100MB, and this hash runs per vault on every daemon-start fan-out (and again on the copy pass). Peak transient memory is ~2× the largest file per sync. Stream the content through the hash in fixed-size chunks so memory stays bounded regardless of file size.

Suggestion:

Suggested change

	hash.update(fs.readFileSync(p));
	hash.update(`f:${r}\n`);
	const fd = fs.openSync(p, 'r');
	try {
	const buf = Buffer.alloc(64 * 1024);
	let n: number;
	while ((n = fs.readSync(fd, buf, 0, buf.length, null)) > 0) {
	hash.update(buf.subarray(0, n));
	}
	} finally {
	fs.closeSync(fd);
	}
	hash.update('\n');

[github-actions]

The tmp dir is created as a sibling of destDir inside the same .claude/skills/ directory the runtime CLIs scan for SKILL.md. During a rebuild, a concurrent CLI scan can see <slug>.tmp-* — and since SKILL.md is copied early, may load a half-copied skill, which is exactly what the atomic-swap comment claims to prevent. Also, if the process is killed mid-copy (SIGKILL/power loss), the orphaned .tmp-* dir persists; for plain-name bundled skills it is never swept by the molio-- orphan cleanup and keeps being picked up by later scans. Consider staging tmp dirs outside the scanned directory and sweeping stale .tmp-*/.bak-* siblings at startup.

[github-actions]

When a rebuild is needed, the source tree is read twice: fully by hashDir (inside isAlreadySynced) and again by copyDirSync. On the every-vault-every-start fan-out this doubles source I/O, which is still costly on NAS-mounted vaults. Consider computing the hash while copying in a single pass, or cheaply short-circuiting on size/mtime mismatch before hashing.

[github-actions]

When vaultId becomes null while a refresh is in flight, loading gets stuck at true forever: the null branch only clears skills and returns, while the in-flight refresh's finally is skipped because vaultIdRef.current !== vaultId. Since the modal stays mounted with a null vaultId (per the hook's own contract), the UI renders an indefinite spinner after the user deselects a vault during a load. Reset loading/error in the null branch too:

Suggestion:

Suggested change

	if (!vaultId) {
	setSkills([]);
	return;
	}
	if (!vaultId) {
	setSkills([]);
	setLoading(false);
	setError(null);
	return;
	}

[github-actions]

Delete does not participate in the openSeqRef sequence guard. The delete button is not disabled while fetchingId === skill.id, so a user can click Edit/Duplicate on a skill, then confirm Delete while api.getSkill is still in flight. When the fetch resolves, the seq check passes (no newer open happened) and the editor opens for a skill that has just been deleted — saving then fails with a 404 (updateSkill on a removed id), or the modal shows content for a skill that no longer exists. Invalidate any in-flight open-fetch when a delete is confirmed: openSeqRef.current += 1; setFetchingId(null); inside handleDeleteConfirm before the API call.

Suggestion:

Suggested change

	const handleDeleteConfirm = useCallback(async (id: string) => {
	setConfirmDeleteId(null);
	try {
	await deleteSkill(id);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [deleteSkill]);
	const handleDeleteConfirm = useCallback(async (id: string) => {
	setConfirmDeleteId(null);
	openSeqRef.current += 1; // invalidate any in-flight edit/duplicate fetch
	setFetchingId(null);
	try {
	await deleteSkill(id);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [deleteSkill]);

[github-actions]

formError is only set on failure in handleToggle/handleDeleteConfirm and never cleared on success. After a failed toggle/delete, the error banner stays visible indefinitely — even after a subsequent action succeeds — because nothing resets formError (only openEdit/openDuplicate/openNew/handleSave/handleImport reset it). Clear formError at the start of these handlers so a stale error doesn't linger.

Suggestion:

Suggested change

	const handleToggle = useCallback(async (id: string, enabled: boolean) => {
	try {
	await toggleSkill(id, enabled);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [toggleSkill]);
	const handleToggle = useCallback(async (id: string, enabled: boolean) => {
	setFormError(null);
	try {
	await toggleSkill(id, enabled);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [toggleSkill]);

[github-actions]

openEdit/openDuplicate await api.getSkill with no abort/unmount guard. SkillsPanel is conditionally mounted ({activeTab === 'skills' && <SkillsPanel />}), so switching tabs while the fetch is pending leaves the resolved promise calling setModal/setFormError/setFetchingId on an unmounted component. React 18 silently drops these, but this is a stale async completion that can trigger act() warnings in tests and would misbehave under other React versions. Invalidate the sequence on unmount (e.g. useEffect(() => () => { openSeqRef.current += 1; }, [])).

Suggestion:

Suggested change

	try {
	const { instructions } = await api.getSkill(skill.id);
	if (openSeqRef.current !== seq) return; // superseded by a newer open
	useEffect(() => () => { openSeqRef.current += 1; }, []);
	const openEdit = useCallback(async (skill: SkillManifestEntry) => {
	const seq = ++openSeqRef.current;
	setFormError(null);
	setFetchingId(skill.id);
	try {
	const { instructions } = await api.getSkill(skill.id);
	if (openSeqRef.current !== seq) return; // superseded by a newer open

[github-actions]

Delete does not participate in the openSeqRef sequence guard. The delete button is not disabled while fetchingId === skill.id, so a user can click Edit/Duplicate on a skill, then confirm Delete while api.getSkill is still in flight. When the fetch resolves, the seq check passes (no newer open happened) and the editor opens for a skill that has just been deleted — saving then fails with a 404 (updateSkill on a removed id), or the modal shows content for a skill that no longer exists. Invalidate any in-flight open-fetch when a delete is confirmed: openSeqRef.current += 1; setFetchingId(null); inside handleDeleteConfirm before the API call.

Suggestion:

Suggested change

	const handleDeleteConfirm = useCallback(async (id: string) => {
	setConfirmDeleteId(null);
	try {
	await deleteSkill(id);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [deleteSkill]);
	const handleDeleteConfirm = useCallback(async (id: string) => {
	setConfirmDeleteId(null);
	openSeqRef.current += 1; // invalidate any in-flight edit/duplicate fetch
	setFetchingId(null);
	try {
	await deleteSkill(id);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [deleteSkill]);

[github-actions]

formError is only set on failure in handleToggle/handleDeleteConfirm and never cleared on success. After a failed toggle/delete, the error banner stays visible indefinitely — even after a subsequent action succeeds — because nothing resets formError (only openEdit/openDuplicate/openNew/handleSave/handleImport reset it). Clear formError at the start of these handlers so a stale error doesn't linger.

Suggestion:

Suggested change

	const handleToggle = useCallback(async (id: string, enabled: boolean) => {
	try {
	await toggleSkill(id, enabled);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [toggleSkill]);
	const handleToggle = useCallback(async (id: string, enabled: boolean) => {
	setFormError(null);
	try {
	await toggleSkill(id, enabled);
	} catch (err) {
	setFormError((err as Error).message);
	}
	}, [toggleSkill]);

[github-actions]

openEdit/openDuplicate await api.getSkill with no abort/unmount guard. SkillsPanel is conditionally mounted ({activeTab === 'skills' && <SkillsPanel />}), so switching tabs while the fetch is pending leaves the resolved promise calling setModal/setFormError/setFetchingId on an unmounted component. React 18 silently drops these, but this is a stale async completion that can trigger act() warnings in tests and would misbehave under other React versions. Invalidate the sequence on unmount (e.g. useEffect(() => () => { openSeqRef.current += 1; }, [])).

Suggestion:

Suggested change

	try {
	const { instructions } = await api.getSkill(skill.id);
	if (openSeqRef.current !== seq) return; // superseded by a newer open
	useEffect(() => () => { openSeqRef.current += 1; }, []);
	const openEdit = useCallback(async (skill: SkillManifestEntry) => {
	const seq = ++openSeqRef.current;
	setFormError(null);
	setFetchingId(skill.id);
	try {
	const { instructions } = await api.getSkill(skill.id);
	if (openSeqRef.current !== seq) return; // superseded by a newer open

[github-actions]

The doc comment above states kind "Defaults to 'library'", but the field is declared optional (kind?). In practice the daemon always populates it — store.ts rowToEntry always sets kind explicitly and the DB schema defaults it to 'library' (NOT NULL DEFAULT 'library') — and the web UI (SkillsPanel/VaultSkillsModal) treats any non-'bundled' value as library. Making the field required (kind: SkillKind) would align the type with both the documented default and the actual wire behavior, and let consumers rely on it without undefined checks.

[github-actions]

VaultSkillEntry duplicates id/name/description/builtIn/kind/createdAt/updatedAt from SkillManifestEntry (the daemon's toVaultSkillEntry in routes/knowledge.ts mirrors them field-by-field). Since this contracts file is the single source of truth for the API surface, consider extracting a shared metadata base type (e.g. interface SkillMetadata { id; name; description; builtIn; kind?; createdAt; updatedAt }) that both SkillManifestEntry and VaultSkillEntry extend. That way future metadata changes (e.g. adding author/version) don't need to be mirrored across two interfaces and the inline comments can't drift out of sync.

[github-actions]

The unfenced path stops consuming field lines at the first blank line (if (!FIELD_LINE.test(line)) break). Real pastes often have blank lines between YAML keys (name: X\n\ndescription: Y), so the description/version get dropped from the frontmatter and the remainder becomes body text — inconsistent with the fenced path, which has no such limitation. Consider allowing a single blank line within the field run (skipping it in the loop) so the unfenced fallback matches the fenced behavior for the same content.

[github-actions]

generateSkillMd hardcodes version: 1.0.0, and parseSkillMd/ParsedSkillMd neither read nor preserve a version field. For the round-trip path (import → edit → save), the original version is silently dropped and reset to 1.0.0 on every generate. If version fidelity matters for the skill format, add version to ParsedSkillMd and re-emit the parsed value (defaulting to 1.0.0); otherwise the hardcoded constant is fine but worth an explicit note in the docs. At minimum, the inconsistency between parsing version as a known field and never surfacing it should be intentional and documented.

[github-actions]

frontmatterField interpolates the key parameter directly into a RegExp constructor (new RegExp(^${key}\s*:\s*(.*)$)) without escaping. All current call sites pass static strings ('name', 'description'), so there is no immediate trigger, but this is a latent regex-injection/disruption hazard if the function is ever reused with dynamic input (e.g. reading a user-specified field name). Escaping the key with key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') would make it safe for any input.

[github-actions]

The unfenced path stops consuming field lines at the first blank line (if (!FIELD_LINE.test(line)) break). Real pastes often have blank lines between YAML keys (name: X\n\ndescription: Y), so the description/version get dropped from the frontmatter and the remainder becomes body text — inconsistent with the fenced path, which has no such limitation. Consider allowing a single blank line within the field run (skipping it in the loop) so the unfenced fallback matches the fenced behavior for the same content.

[github-actions]

generateSkillMd hardcodes version: 1.0.0, and parseSkillMd/ParsedSkillMd neither read nor preserve a version field. For the round-trip path (import → edit → save), the original version is silently dropped and reset to 1.0.0 on every generate. If version fidelity matters for the skill format, add version to ParsedSkillMd and re-emit the parsed value (defaulting to 1.0.0); otherwise the hardcoded constant is fine but the inconsistency between parsing version as a known field and never surfacing it should be intentional and documented.

[github-actions]

frontmatterField interpolates the key parameter directly into a RegExp constructor (new RegExp(^${key}\s*:\s*(.*)$)) without escaping. All current call sites pass static strings ('name', 'description'), so there is no immediate trigger, but this is a latent regex-injection/disruption hazard if the function is ever reused with dynamic input (e.g. reading a user-specified field name). Escaping the key with key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') would make it safe for any input.

[github-actions]

🔍 OpenCodeReview found 52 issue(s) in this PR.

• ✅ 52 posted as inline comment(s)
• 📝 0 posted as summary

---

📊 Posting Statistics:

• ✅ Successfully posted: 52 comment(s)

[github-actions]

🔍 OpenCodeReview found 47 issue(s) in this PR.

• ✅ 47 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

Initialization-order issue: maybeCreateDefaultVault runs at server.ts module-evaluation time (top-level call), which is BEFORE initSkillLibrary(db) seeds the skills table in index.ts. On a fresh install the table is still empty when reconcileVault runs here, so this creation-time sync is effectively a no-op (empty effective set — reconcileSync([]) + reconcileBundledSync(∅, ∅, …), and ensureMolioRules would even strip gated rules if a CLAUDE.md pre-exists in the mount). The vault's skills only get populated later by reconcileAllVaultsAsync in the post-listen startup chores — and index.ts skips that fan-out entirely when seeding fails (if (skillsSeeded)). The old installBuiltinSkills installed built-ins directly from disk regardless of the DB, so a seeding failure now leaves the freshly-created default vault with NO skills. Consider seeding before default-vault creation (or passing the seeded state into this path) so the vault is reconciled against a populated table.

[github-actions]

isValidSkillId is documented as the single choke point guaranteeing that "no caller (DB row, route param, imported filename) can ever escape the skills dirs", but the validation is incomplete for edge-case IDs that would still reach path.join. It does not reject NUL bytes (\0), which make path.join throw ERR_INVALID_ARG_VALUE on every platform (a 500/DoS instead of a clean rejection); it also allows Windows-invalid characters (< > : " | ? *), reserved device names (CON, PRN, AUX, NUL, COM1-9, LPT1-9), and trailing dots/spaces (silently stripped by Windows, causing path mismatches). Additionally, id.length <= 128 counts UTF-16 code units, so a 128-char multi-byte ID (e.g. emoji) plus the molio-- prefix can exceed the ~255-byte filename limit → ENAMETOOLONG. Since the daemon is cross-platform and this module is the stated safety invariant, consider also rejecting \0, forbidden Windows chars/reserved names, and enforcing a byte-length cap via Buffer.byteLength(id).

[github-actions]

scratchDir returns a single shared ~/.molio/scratch used as the cwd for every prefill run. The daemon is a plain HTTP server, so concurrent /api/skills/prefill requests (not just the bundled UI, which has its own busy-guard) will run Claude with the same cwd — any file the agent writes there can collide with or be picked up by another run, and there is no cleanup of whatever the agent leaves behind. Consider isolating per run (e.g. scratch/<runId>) and cleaning it up in settle.

[github-actions]

fs.readFileSync (and the preceding fs.statSync for the single-file branch) sits outside any SkillImportError conversion. If the file is deleted/renamed between resolveSource's stat and this read, or becomes unreadable (EACCES), the raw ENOENT/EPERM propagates to the route's generic catch-all and surfaces as a 500 with a low-level fs message instead of a clean 400/404. Wrap the fs boundary reads in try/catch and convert to SkillImportError.

Suggestion:

Suggested change

	const raw = fs.readFileSync(src.skillMd, 'utf8');
	let raw: string;
	try {
	raw = fs.readFileSync(src.skillMd, 'utf8');
	} catch {
	throw new SkillImportError('NOT_FOUND', `无法读取文件：${src.skillMd}`);
	}

[github-actions]

The recursive walk calls fs.readdirSync on every subdirectory without a guard. An unreadable subtree (EACCES/EPERM on a protected folder) throws a raw system error that escapes the SkillImportError mapping and yields a 500. Consider skipping unreadable dirs (consistent with copyDirSync's tolerance) or converting to a clean SkillImportError.

Suggestion:

Suggested change

	for (const entry of fs.readdirSync(d, { withFileTypes: true })) {
	let entries: fs.Dirent[];
	try {
	entries = fs.readdirSync(d, { withFileTypes: true });
	} catch {
	return;
	}
	for (const entry of entries) {

[github-actions]

The COLLAPSED_FIELD lookahead splits on whitespace before ANY occurrence of name:/description:/version:, whether or not that text is a real field declaration. A value such as description: 支持 version: 2 的 API will be split into description: 支持 + version: 2 的 API, truncating the description and injecting a junk field. The existing test only guards the see: (non-known key) case, so values that legitimately mention one of these three key names are silently corrupted after round-tripping through the editor/importer. Consider restricting the split to lines that start with a known skill field and only splitting when the split actually separates well-formed known fields (or quoting/escaping values), so ordinary prose mentioning these keys isn't mangled.

[github-actions]

The fenced-frontmatter regex ^---...\n--- also matches a plain markdown document that happens to start with a --- horizontal rule and contains another --- later. Everything between the two rules is then treated as frontmatter and dropped from the instructions (silent content loss when the result is saved/imported), and any name:/description: text in that region is misread as metadata. Since the module is explicitly tolerant of missing fences, consider only treating the block as frontmatter when it actually contains at least one known field line (name:/description:/version:), otherwise falling through to the plain-body path.

[github-actions]

name/description are interpolated into the YAML frontmatter without any escaping. User-controlled values containing : (e.g. React: 教程), quotes, or leading #/%/@ produce invalid or ambiguous YAML for strict parsers. The generated SKILL.md is also consumed by runtime CLIs' skill discovery (per the file header), so such values can break external tooling even though this app's own tolerant parser round-trips them. Consider quoting/escaping the values (or validating at input) so the emitted frontmatter stays parseable by external consumers.

[github-actions]

Focus outline removed on form fields: .sk-field__input:focus, .sk-field__textarea:focus { outline: none; } leaves only a border-color change as the focus indicator, which is hard to notice (especially for keyboard/Tab users). This also conflicts with the project's established pattern — the same diff's .sk-switch input:focus-visible and existing .settings-tab-btn:focus-visible / base.css button:focus-visible keep visible outlines. Use :focus-visible with a visible indicator (e.g. outline: 2px solid var(--accent); outline-offset: 2px;) to preserve keyboard accessibility.

Suggestion:

Suggested change

	.sk-field__input:focus,
	.sk-field__textarea:focus {
	outline: none;
	border-color: var(--accent);
	}
	.sk-field__input:focus-visible,
	.sk-field__textarea:focus-visible {
	outline: 2px solid var(--accent);
	outline-offset: 2px;
	border-color: var(--accent);
	}

[github-actions]

Focus outline removed on form fields: .sk-field__input:focus, .sk-field__textarea:focus { outline: none; } leaves only a border-color change as the focus indicator, which is hard to notice (especially for keyboard/Tab users). This also conflicts with the project's established pattern — the same diff's .sk-switch input:focus-visible and existing .settings-tab-btn:focus-visible / base.css button:focus-visible keep visible outlines. Use :focus-visible with a visible indicator (e.g. outline: 2px solid var(--accent); outline-offset: 2px;) to preserve keyboard accessibility.

Suggestion:

Suggested change

	.sk-field__input:focus,
	.sk-field__textarea:focus {
	outline: none;
	border-color: var(--accent);
	}
	.sk-field__input:focus-visible,
	.sk-field__textarea:focus-visible {
	outline: 2px solid var(--accent);
	outline-offset: 2px;
	border-color: var(--accent);
	}

[github-actions]

🔍 OpenCodeReview found 42 issue(s) in this PR.

• ✅ 42 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

The limit walk counts symlink targets with fs.statSync (which dereferences links), but the actual copy — copyDirSync in dirsync.ts — SKIPS symlinks (entry.isSymbolicLink() → skip). So the comment "follows symlinks, like copyDirSync does" is inaccurate, and the two rules diverge functionally:

1. A skill folder whose reference/script siblings are symlinks (common in git-style repos) passes the limits but imports with those files silently missing — the SKILL.md references break in the library and in every synced vault copy.
2. A symlink to a large external file is counted toward MAX_IMPORT_BYTES and can falsely reject an import even though the copy would never include it.
   Recommend aligning the walk's rule with the copy (skip links) or explicitly documenting the divergence so the limits and the actual copied content stay consistent.

[github-actions]

fs errors at the import boundary aren't wrapped into SkillImportError. resolveSource turns EVERY stat failure (including EACCES on a protected path, e.g. macOS TCC-guarded folders) into a "not found" → misleading 404. Meanwhile readdirSync/statSync/readFileSync inside assertFolderWithinLimits/importFromFolder (unreadable subdir, file deleted between the stat and the read) throw raw Node errors that escape the SkillImportError branch in routes/skills.ts and surface as a generic 500 INTERNAL with an OS error message. Map permission/I/O failures to SkillImportError (e.g. a distinct code or BAD_REQUEST) so the API returns a consistent structured 400/404 instead of leaking raw fs exceptions.

[github-actions]

Age is determined from the directory's own mtime, which only changes when a direct child is created/deleted — appending to an existing events.jsonl does not touch it. A run directory older than maxAgeDays whose log stream is still being appended (a long-lived multi-turn session, or a child process orphaned across a daemon restart) is classified as expired and rmSync'd mid-write. The old sync sweep ran before the server accepted connections, so it could never hit an active log; pruneRunLogsAsync now yields to the event loop right after listen, so HTTP requests can create/continue runs while the sweep is in progress. Consider judging age from the newest file inside the directory (e.g. events.jsonl's mtime) or skipping directories that belong to currently active runs.

[github-actions]

pruneEntry uses fs.statSync, which follows symlinks. A symlink inside the runs dir that points to a directory is treated as a run-log directory; if the target's mtime is stale, rmSync removes the symlink itself (rmSync does not follow the link) and counts it as 'removed'. The daemon never creates such links, but the sweep already accounts for symlinks in its tests (dangling symlink case), so classifying them as keepable is the safer default — lstatSync would treat any symlink as a non-directory and leave it untouched.

[github-actions]

On a fresh headless boot, maybeCreateDefaultVault runs during server.ts module evaluation, which is hoisted ahead of index.ts's initSkillLibrary(db) seed (the startup-order test only pins seeding before listen). At provisioning time the skills table is therefore empty: reconcileVault sees no effective/managed skills, so reconcileBundledSync(∅, ∅, …) installs nothing and the gated CLAUDE.md rules (docling/remotion/wiki-query) are not added. The provisioning-time sync becomes a no-op on first boot — unlike the old installBuiltinSkills, which had no DB dependency and installed reliably. Skills only appear after the post-listen reconcileAllVaultsAsync fan-out; if the process is killed before that completes, the new vault stays skill-less until the next restart. Consider seeding the library before maybeCreateDefaultVault runs (e.g. move initSkillLibrary into server.ts before provisioning), or drop the provisioning-time reconcile and rely solely on the deferred fan-out.

[github-actions]

All fields are optional, so an empty object {} is a valid UpdateSkillRequest. The daemon PATCH handler only checks for a non-null body and forwards it to updateSkill, which then performs a no-op write: it bumps updatedAt and rewrites SKILL.md even when no field actually changed. Consider validating that at least one field is present (e.g., reject empty patches in the route, or express the constraint in the type) to avoid pointless DB writes and file rewrites.

[github-actions]

refreshSeqRef only guards refresh-vs-refresh races, not refresh-vs-mutation races. desiredRef is deleted as soon as the latest toggle response lands, so a refresh that started before a mutation but resolves after it will overwrite the correct state: the list request was served before the daemon processed the toggle, and applyDesired no longer has an overlay entry to correct it (UI shows the stale enabled value while the daemon holds the new one). The same hole lets a deleted skill reappear (a stale listSkills response replaces the whole array, undoing setSkills(prev => prev.filter(...))), and lets createSkill/updateSkill/importSkill results be clobbered until the next explicit refresh — exactly the class of bug this hook's comments claim to prevent. Consider a mutation epoch: bump a counter on every mutation and ignore refresh responses whose seq predates the latest mutation, or skip the setSkills write when any mutation committed while the request was in flight.

[github-actions]

toggleSeqRef entries are never removed for the hook's lifetime, and — more importantly — if an api.toggleSkill promise never settles (hanging network / daemon stuck in vault re-sync), its desiredRef entry is never deleted, so applyDesired keeps overlaying the stale optimistic value on every future refresh() indefinitely, leaving a permanently wrong enabled state for that skill. Suggest removing the seq entry once the toggle settles and guarding against never-settling promises (e.g., a timeout or cleanup on unmount) so a stale desiredRef can't persist a wrong state.

[github-actions]

COLLAPSED_FIELD splits any frontmatter line at whitespace preceding the literal substrings name:, description:, or version:. A legitimate value containing such text — e.g. a name My description: X or a description Prints the name: value pair — is split into two field lines, silently truncating the value and fabricating a spurious field. Because generateSkillMd emits these fields unquoted, one per line, the generate→parse round-trip relied on by store.ts writeSkillMd/updateSkill, the importer, and the web editor is not idempotent for such values. The existing test only guards unknown keys (see:), which never match this pattern. Suggestion: only apply the collapsed-field split when the line starts with a known field key AND the value segment before the boundary is plausibly short (no internal spaces), or quote values in generateSkillMd and skip splitting inside quoted values.

[github-actions]

The docstring states emoji "don't split mid-character", but Array.from iterates by Unicode code point, not grapheme cluster. ZWJ sequences (👨‍👩‍👧), flag emoji (regional-indicator pairs), and variation-selector sequences span multiple code points and will be cut mid-sequence, producing a broken auto-derived name. Use Intl.Segmenter with grapheme granularity to honor the documented guarantee.

Suggestion:

Suggested change

	function firstChars(text: string, n: number): string {
	const chars = Array.from(text);
	return singleLine(chars.slice(0, n).join(''));
	}
	function firstChars(text: string, n: number): string {
	const segmenter = new Intl.Segmenter(undefined, { granularity: 'grapheme' });
	const graphemes = Array.from(segmenter.segment(text), (s) => s.segment);
	return singleLine(graphemes.slice(0, n).join(''));
	}

[github-actions]

Affected Areas & Test Selection

Matched areas:

• chat
• kb
• navigation
• settings
• desktop
• kb-import

Changed files: 57

• apps/daemon/CLAUDE.md
• apps/daemon/src/core/db.ts
• apps/daemon/src/core/default-vault.ts
• apps/daemon/src/core/runs-log-prune.ts
• apps/daemon/src/core/skill-installer.ts
• apps/daemon/src/core/skills/builtin.ts
• apps/daemon/src/core/skills/dirsync.ts
• apps/daemon/src/core/skills/importer.ts
• apps/daemon/src/core/skills/paths.ts
• apps/daemon/src/core/skills/prefill.ts
• apps/daemon/src/core/skills/store.ts
• apps/daemon/src/core/skills/sync.ts
• apps/daemon/src/core/skills/vault-config.ts
• apps/daemon/src/index.ts
• apps/daemon/src/routes/knowledge.ts
• apps/daemon/src/routes/skills.ts
• apps/daemon/src/server.ts
• apps/daemon/test/compat/daemon-startup-order.test.ts
• apps/daemon/test/core/default-vault.test.ts
• apps/daemon/test/core/runs-log-prune.test.ts
• ... and 37 more

Top changed symbols (by caller count):

Symbol	Kind	Callers
copyDirSync	function	3
pruneRunLogsAsync	function	1
BUILTIN_SKILLS	const	1
resolveSkillsSourceDir	function	1
installSkills	method	0
reconcileSkills	method	0
pruneEntry	function	0
logPruneSummary	function	0
logFn	method	0
readPruneEntries	function	0
ASYNC_CHUNK_SIZE	const	0
isBuiltinSkillsDir	function	0
readSkillVersion	function	0
shouldUpdateSkill	function	0
DEPRECATED_SKILLS	const	0

E2E specs that will run on this PR:

• bootstrap.spec.ts (P0)
• chat-codeblock.spec.ts (P0)
• chat-copy.spec.ts (P0)
• chat-delete.spec.ts (P0)
• chat-edit-resend.spec.ts (P0)
• chat-multi.spec.ts (P0)
• chat-overflow-menu.spec.ts (P0)
• chat-regenerate.spec.ts (P0)
• chat-repairing.spec.ts (P1)
• chat-single.spec.ts (P0)
• chat-streaming.spec.ts (P1)
• create-vault-form.spec.ts (P0)
• drag-drop-import.spec.ts (P1)
• kb-chat-entry.spec.ts (P1)
• kb-file-drag-real.spec.ts (P1)
• kb-folder-drag-real.spec.ts (P1)
• kb-folder-ops.spec.ts (P1)
• kb-folder-root-drop.spec.ts (P1)
• kb-large-file-15mb.spec.ts (P0)
• kb-large-file-gbk.spec.ts (P0)
• kb-large-file.spec.ts (P0)
• kb-qa-trigger.spec.ts (P1)
• kb-tab-limit.spec.ts (P0)
• kb-tab-stale.spec.ts (P1)
• kb-tree-sort-locate.spec.ts (P1)
• knowledge.spec.ts (P0)
• markdown-render.spec.ts (P1)
• navigation.spec.ts (P1)
• settings.spec.ts (P1)
• skills.spec.ts (P1)

---

Generated by Impact Analysis workflow. This is informational, not a gate.

[github-actions]

🔍 OpenCodeReview found 45 issue(s) in this PR.

• ✅ 45 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

Startup-order issue: the default reconcileSkills is reconcileVault, which reads the skills table via getEffectiveSkills/listSkills. But maybeCreateDefaultVault runs during server.ts's module-body evaluation (triggered by index.ts's import ... from './server.js'), which happens BEFORE initSkillLibrary(db) seeds that table at index.ts line 99. On a fresh containerized first boot, this reconcile therefore sees an empty skill library: bundled skills (docling/wiki-*/remotion/wechat-article-extractor, which are always-on app-owned functionality) and gated CLAUDE.md rules are only installed later by the deferred reconcileAllVaultsAsync — and if seeding fails (or the daemon dies in that window), the vault is left without the bundled skills that installBuiltinSkills used to guarantee unconditionally. The old code read from the filesystem source dir and had no such DB dependency. Consider seeding the skill library before the module-level vault provisioning (e.g. move the maybeCreateDefaultVault call out of server.ts's module body, or seed within openDatabase/before it).

[github-actions]

refreshMeta only updates the DB row's name/description — it never regenerates the on-disk ~/.molio/skills/<id>/SKILL.md for core skills. Since core skills are hidden and not editable (routes/skills.ts 404s them), a future release that improves CORE_SKILLS_SEEDS will never reach existing installs: the DB row looks refreshed, but the file actually synced into every vault (sync.ts mirrors the content dir, whose frontmatter carries name/description and whose body carries the instructions) stays frozen at the first-seed version. This is asymmetric with bundled skills, whose content updates propagate live from app resources. Consider regenerating the core skill's SKILL.md when the seed changes (or version-gate the refresh).

[github-actions]

Check-then-insert race: getSkill followed by createSkill is non-atomic, and createSkill uses a plain INSERT (id is the PRIMARY KEY). If two daemon processes seed concurrently (restart overlap, or a test suite sharing the DB file), both can read 'no row' and one will hit the UNIQUE constraint, throwing out of seedBuiltinSkills → initSkillLibrary returns false → the caller skips the entire per-vault fan-out for that start. Consider an upsert (INSERT OR IGNORE / ON CONFLICT DO NOTHING) followed by re-fetch, or wrap the seed in a transaction, so a duplicate seed is a no-op instead of a total-seed failure.

[github-actions]

The swap sequence (copy → rename dest→bak → rename tmp→dest → rm bak) has no cross-process exclusion, yet the module explicitly anticipates other processes (see sweepStaleMirrorArtifacts' "another process is building right now" comment). Within one daemon, reconcileVault is synchronous so calls serialize, but two daemon instances can sync the same vault: overlapping startup where checkAndKillPortOccupant kills the old daemon while the new one begins its deferred fan-out, or two machines pointed at a NAS-backed vault. Interleaved renames can make one process's renameSync(destDir, backup) throw ENOENT (the other process already moved dest) → the mirror fails and its staging dir is discarded; worse, on POSIX a second process's renameSync(tmp, destDir) atomically replaces the dest the first process just swapped in — and the first process already deleted its backup — silently downgrading a freshly synced (possibly newer-upgrade) revision. Consider a per-vault lock file (open with 'wx' + stale takeover) or at least an existsSync(destDir) re-check immediately before the final rename, treating a moved/regained dest as a conflict.

[github-actions]

sweepStaleMirrorArtifacts judges staleness from the staging dir's mtime, but a directory's mtime is only bumped when entries are created/removed — NOT while a single large file's bytes stream in. A skill file can be up to ~100MB (the import cap) and copyDirSync copies it via copyFileSync; on slow NAS storage a copy can exceed STALE_ARTIFACT_MS (5 min), so a concurrent sweep (the multi-process case this module already anticipates) would rm -rf the in-flight .tmp-* dir mid-copy, failing the mirror with ENOENT and discarding the half-copied staging content instead of waiting. Consider a heartbeat (touch a marker file while copying) or requiring the artifact to be both older than the cutoff AND not modified-recently at deletion time.

[github-actions]

On a failed toggle, void refresh() synchronously sets loading=true and error=null, so the entire list is swapped out for a full-list spinner even though only a single toggle failed (SkillsPanel renders loading ? spinner : list), and the hook-level error is cleared. The error is only preserved because the caller rethrows it. Consider re-syncing without flipping loading (e.g., a background refresh that keeps the current list visible) so a single toggle failure doesn't cause a full-list loading flash.

[github-actions]

The global input:focus, textarea:focus rule in base.css sets box-shadow: 0 0 0 3px var(--selected-soft) (blue ring). This local rule only overrides outline and border-color, so the --selected/--selected-soft blue ring from base.css still renders alongside the new coral --accent border — two different color systems fighting on the same focus state. The focus indicator is still visible (so it's not an a11y gap), but the mixed colors look inconsistent. Either keep the base focus (drop the local border-color override) or also set an accent-based ring for a coherent focus: box-shadow: 0 0 0 3px var(--accent-soft).

[github-actions]

generateSkillMd interpolates name/description into the frontmatter without YAML escaping. A value containing : (e.g. a description like "保存为技能: 管理文件" or "Note: this skill..."), a leading #, or quotes produces frontmatter that strict YAML parsers (the runtime CLIs doing skill discovery, per the file's header) reject or misparse — the skill can silently fail to load or get a wrong name/description. singleLine only collapses newlines; it doesn't make the value a valid YAML scalar. Consider emitting values as double-quoted scalars (e.g. JSON.stringify, which is valid YAML for most strings) or otherwise escaping them.

Suggestion:

Suggested change

	return (
	`---\n` +
	`name: ${singleLine(name)}\n` +
	`description: ${singleLine(description)}\n` +
	function yamlQuote(value: string): string {
	return JSON.stringify(singleLine(value));
	}
	export function generateSkillMd(name: string, description: string, instructions: string): string {
	return (
	`---\n` +
	`name: ${yamlQuote(name)}\n` +
	`description: ${yamlQuote(description)}\n` +

[github-actions]

COLLAPSED_FIELD splits a line at any whitespace preceding a known key, even when that key text is genuinely part of the preceding value. E.g. description: 用法 name: 管理文件 is re-split into description: 用法 + name: 管理文件, truncating the description and creating a spurious name field. The existing tests only cover non-known keys (see:) inside values, so this false positive is untested. Consider restricting the split (e.g. only treat it as a collapsed field when the preceding segment is short/plausible for the previous field) or at least documenting this trade-off.

[github-actions]

In the unfenced path, the consume loop tests every contiguous line against FIELD_LINE (^word:), not just the known skill keys, even though the block is only entered when the FIRST line is a known field. So a body that starts immediately with a word: line and no blank separator (e.g. name: x\ndescription: d\nNote: 正文第一行) has that body line silently swallowed into the frontmatter and dropped from the instructions. Gating the loop on KNOWN_FIELD (name/description/version) instead of FIELD_LINE would keep only real skill fields in the frontmatter and leave word: body lines intact.

[github-actions]

🔍 OpenCodeReview found 38 issue(s) in this PR.

• ✅ 38 posted as inline comment(s)
• 📝 0 posted as summary

[github-actions]

fs.readdirSync(d, ...) in the recursive walk has no error handling. If a subdirectory is unreadable (EACCES/EPERM on permissioned dirs, broken mounts) or disappears mid-walk, a raw Node fs error propagates out of assertFolderWithinLimits and, since it isn't a SkillImportError, the route converts it to a generic 500 instead of a friendly 400. Wrap the readdirSync in try/catch and rethrow as SkillImportError, mirroring how the statSync below already degrades gracefully.

Suggestion:

Suggested change

	const walk = (d: string): void => {
	for (const entry of fs.readdirSync(d, { withFileTypes: true })) {
	const walk = (d: string): void => {
	let entries: fs.Dirent[];
	try {
	entries = fs.readdirSync(d, { withFileTypes: true });
	} catch {
	throw new SkillImportError('BAD_REQUEST', `无法读取子目录：${d}`);
	}
	for (const entry of entries) {

[github-actions]

The comment claims statSync follows symlinks "like copyDirSync does", but copyDirSync (dirsync.ts) actually SKIPS symlinks (entry.isSymbolicLink() → skip). Because of this mismatch, a directory whose root SKILL.md is a symlink passes every check here — existsSync, statSync, and readFileSync all follow the link — yet the symlink is omitted during the copy. The imported skill's content dir then ends up with no SKILL.md, producing a skill that syncs to vaults without a discoverable SKILL.md (broken import). Consider rejecting symlinks during the walk, or copying the resolved target so the guard and the copy stay consistent.

Suggestion:

Suggested change

	size = fs.statSync(p).size; // follows symlinks, like copyDirSync does
	size = fs.statSync(p).size; // follows symlinks; copyDirSync skips symlinks, so reject them here

[github-actions]

The path-traversal guard rejects /, \, and exact ./.., but on Windows a trailing dot or trailing space is silently stripped by the Win32 filesystem, so IDs like .. , ..., or a. normalize to .., ..., a once interpolated into a path. Since this module is the single owner of the path layout (skillContentDir/molioSkillDir feed mkdirSync/writeFileSync/rmSync), closing the gap here — e.g. rejecting ids that end with a dot/space or a trailing sequence of dots, and (for completeness) Windows-reserved names (CON, NUL, COM1, …) and chars < > : " | ? * — keeps the invariant airtight even if a future caller (route param, imported filename) feeds it user-controlled ids. Currently the only reachable ids are UUIDs and hardcoded slugs, so this is defense-in-depth, not an active exploit.

Suggestion:

Suggested change

	id !== '.' &&
	id !== '..' &&
	!id.includes('/') &&
	!id.includes('\\') &&
	id === path.basename(id)
	id !== '.' &&
	id !== '..' &&
	!/[\. ]$/.test(id) &&
	!id.includes('/') &&
	!id.includes('\\') &&
	id === path.basename(id)

[github-actions]

Ordering hazard: maybeCreateDefaultVault runs at module scope of server.ts, which ESM evaluates before index.ts's top-level initSkillLibrary(db) (index.ts imports server.js). So this provisioning-time reconcile reads an UNSEEDED (empty) skills table on first boot. With an empty table, reconcileVault → reconcileSync([]) performs orphan cleanup and treats every existing molio--<id> dir as an orphan. In the exact deployment this feature targets — persistent /vaults NAS mount + ephemeral container DB (DB wiped on recreate, mount preserved) — previously synced library/core skills get DELETED here before the table is seeded, then re-created only if the deferred reconcileAllVaultsAsync runs and seeding succeeded. index.ts's own comment establishes the invariant "seeding must run before any vault reconcile reads the table" — this path violates it. Suggest seeding the skills table before provisioning (or skipping the reconcile here when the table is empty).

[github-actions]

Failed-rollback path can permanently lose the deployed skill. When the final rename(tmp, destDir) fails and the restore rename(backup, destDir) ALSO fails (the inner catch swallows the restore error and rethrows the original), the old content stays parked under destDir.bak-*. That backup name matches MIRROR_ARTIFACT_RE and is handed to sweepStaleMirrorArtifacts, which will delete it after 5 minutes — so a transient EPERM/AV-lock failure converts the intended 'old content still present' degradation into 'skill dir gone'. The restore failure should at minimum be surfaced/logged distinctly, and the orphaned backup should be preserved (e.g. excluded from the sweep) rather than being eligible for deletion.

[github-actions]

generateSkillMd inserts name/description into YAML frontmatter without quoting or escaping. A value that is a bare block-scalar indicator (|, >, |-, >+) or starts with one is emitted as e.g. description: | and re-parsed as an empty block scalar — the value is lost entirely on round-trip. Values wrapped in quotes ("...") are also stripped by the parser's quote-removal. Since the web editor saves through this function, editing a skill whose name/description contains such characters silently corrupts its metadata. Consider quoting YAML values (and escaping embedded quotes) or rejecting block-scalar indicator values at generation time.

Suggestion:

Suggested change

	export function generateSkillMd(name: string, description: string, instructions: string): string {
	return (
	`---\n` +
	`name: ${singleLine(name)}\n` +
	`description: ${singleLine(description)}\n` +
	`version: 1.0.0\n` +
	`---\n\n` +
	`${instructions.trim()}\n`
	);
	}
	export function generateSkillMd(name: string, description: string, instructions: string): string {
	return (
	`---\n` +
	`name: ${singleLine(name)}\n` +
	`description: ${singleLine(description)}\n` +
	`version: 1.0.0\n` +
	`---\n\n` +
	`${instructions.trim()}\n`
	);
	}

[github-actions]

version: 1.0.0 is hardcoded on every generated SKILL.md, and parseSkillMd never reads the version field. Editing an imported skill that carries a different version (e.g. version: 2.3.0) resets it to 1.0.0, and the version is silently dropped from the parsed result. The ParsedSkillMd interface has no version field, so version information can never round-trip. If version is meaningful for vault-installed skills (as the field's presence in the format suggests), consider parsing and preserving it rather than unconditionally rewriting it.

Suggestion:

Suggested change

	`version: 1.0.0\n` +
	`version: 1.0.0\n` +

[github-actions]

firstHeadingText toggles the fence state on ANY line starting with triple backticks/tildes, with no check that the fence is properly opened/closed. An unclosed fence (common in pasted snippets) or a stray fence line in prose leaves the state inverted, so genuine headings are skipped — or a code-comment line (# comment inside a fence) is selected as the skill name instead. The resulting fallback name is wrong and the failure is silent. Consider tracking the fence's info-string/backtick count so only a matching closing fence toggles the state, and treat an unclosed fence as terminating at end of input.

Suggestion:

Suggested change

	let inFence = false;
	for (const line of body.split(/\r?\n/)) {
	if (/^\s*(```|~~~)/.test(line)) {
	inFence = !inFence;
	continue;
	}
	if (inFence) continue;
	let inFence = false;
	for (const line of body.split(/\r?\n/)) {
	if (/^\s*(```|~~~)/.test(line)) {
	inFence = !inFence;
	continue;
	}
	if (inFence) continue;

[github-actions]

This .sk-row--off state is dead CSS — searching the entire codebase shows the class is never applied by any component (SkillsPanel.tsx always renders .sk-row without the --off modifier). The comment describes a 'switch locked off (per-vault modal)' behavior that isn't wired up anywhere in this change set. Either implement the locked-off state in the component that renders the rows, or remove these rules to avoid dead styles that imply behavior which doesn't exist.

[github-actions]

.sk-field--grow .sk-field__textarea (specificity 0,2,0) sets min-height: 0, which silently overrides the min-height: 160px from .sk-field__textarea--md (specificity 0,1,0) on the instructions textarea in the fullscreen editor (it carries both classes). On short viewports the grow textarea can shrink to a near-zero height, making the main authoring surface unusable. If the 160px minimum is still desired for usability in the fullscreen layout, keep a floor (e.g. min-height: 160px in the grow rule) or drop the --md class there to make the intent explicit.