From 310524ddd64af45b7d857c295236ec2fa9007b3d Mon Sep 17 00:00:00 2001 From: Scot Campbell Date: Fri, 17 Apr 2026 12:25:11 -0400 Subject: [PATCH 1/2] fix: scan rules from disk; dedup flat-md scanners; round-trip paths/tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rules in ~/.claude/rules/ and {project}/.claude/rules/ never appeared in the UI because no scanner populated the `rules` table — it was only ever written by in-UI create_rule. Every other behavioural primitive has both global and project scanners; this closes the scope asymmetry. Changes: * utils/paths.rs: add rules_dir to ClaudePathsInternal. * services/scanner.rs: - Introduce SOURCE_AUTO_DETECTED constant and walk_md_dir helper. - Migrate scan_global_commands, scan_global_agents, scan_project_commands, and scan_project_agents to use the helper — behaviour unchanged. - Add ParsedRule, parse_rule_file, parse_list_value (accepts both JSON- array and legacy comma-separated forms), upsert_rule_from_file, scan_global_rules, scan_project_rules. Populate is_symlink and symlink_target (previously dead-lettered). Wire into run_startup_scan and the per-project loop. * services/rule_writer.rs: emit paths and tags as JSON arrays so they round-trip through the DB reader (which already expects JSON). The previous comma-joined form was lost to serde_json::from_str. Tags were dropped entirely before; now they round-trip too. * commands/rules.rs: delete_rule now also removes the backing .md file, so a UI delete is not resurrected by the new scanner as 'auto-detected'. Factor the logic behind delete_rule_inner(conn, id, home) for testing. Not touched (different shapes, out of scope): * scan_*_skills (directory-based with skill_files child table). * scan_*_hooks (JSON inside settings.json). * Vendor scanners (opencode/codex/copilot/cursor/gemini). Tests: * 18 new tests across the four files (walk_md_dir edge cases, parse_rule_file, parse_list_value dual-form, scan_global_rules insert/ idempotent/backfill/symlink, scan_project_rules assignment, rule_writer JSON emission for paths and tags, delete_rule_inner with disk-present and disk-absent). * Full suite: 2010 pass, 0 fail. --- src-tauri/src/commands/rules.rs | 99 +++- src-tauri/src/services/config_writer.rs | 14 + src-tauri/src/services/rule_writer.rs | 24 +- src-tauri/src/services/scanner.rs | 709 +++++++++++++++++++----- src-tauri/src/utils/paths.rs | 3 + 5 files changed, 710 insertions(+), 139 deletions(-) diff --git a/src-tauri/src/commands/rules.rs b/src-tauri/src/commands/rules.rs index c8de353e..7510f583 100644 --- a/src-tauri/src/commands/rules.rs +++ b/src-tauri/src/commands/rules.rs @@ -139,13 +139,36 @@ pub fn update_rule( stmt.query_row([id], row_to_rule).map_err(|e| e.to_string()) } +/// Inner helper for [`delete_rule`]: drops the DB row and best-effort +/// removes the backing `.md` file under `{home}/.claude/rules/.md`. +/// Factored out so tests can pass a temp dir in place of `~`. +fn delete_rule_inner(conn: &rusqlite::Connection, id: i64, home: &Path) -> Result<(), String> { + let query = format!("SELECT {} FROM rules WHERE id = ?", RULE_SELECT_FIELDS); + let rule: Option = conn + .prepare(&query) + .ok() + .and_then(|mut s| s.query_row([id], row_to_rule).ok()); + + conn.execute("DELETE FROM rules WHERE id = ?", [id]) + .map_err(|e| e.to_string())?; + + if let Some(rule) = rule { + // Best-effort — if the file is already gone (user deleted by hand) we + // still want the DB delete to succeed. + let _ = rule_writer::delete_rule_file(home, &rule); + } + + Ok(()) +} + #[tauri::command] pub fn delete_rule(db: State<'_, Arc>>, id: i64) -> Result<(), String> { let db = db.lock().map_err(|e| e.to_string())?; - db.conn() - .execute("DELETE FROM rules WHERE id = ?", [id]) - .map_err(|e| e.to_string())?; - Ok(()) + let base_dirs = + directories::BaseDirs::new().ok_or_else(|| "Could not find home directory".to_string())?; + // Without the disk delete, the next startup scan would resurrect the rule + // as `source='auto-detected'`. + delete_rule_inner(db.conn(), id, base_dirs.home_dir()) } #[tauri::command] @@ -548,4 +571,72 @@ mod tests { assert!(glob_match("src/*.ts", "src/index.ts")); assert!(!glob_match("src/*.ts", "src/deep/index.ts")); } + + // ========================================================================= + // delete_rule_inner tests + // ========================================================================= + + fn setup_test_db() -> Database { + Database::in_memory().unwrap() + } + + #[test] + fn test_delete_rule_removes_disk_file() { + let db = setup_test_db(); + let temp_dir = tempfile::TempDir::new().unwrap(); + + db.conn() + .execute( + "INSERT INTO rules (name, content) VALUES (?, ?)", + params!["my-rule", "body"], + ) + .unwrap(); + let id = db.conn().last_insert_rowid(); + + let rules_dir = temp_dir.path().join(".claude").join("rules"); + std::fs::create_dir_all(&rules_dir).unwrap(); + let file_path = rules_dir.join("my-rule.md"); + std::fs::write(&file_path, "body").unwrap(); + assert!(file_path.exists()); + + delete_rule_inner(db.conn(), id, temp_dir.path()).unwrap(); + + assert!(!file_path.exists(), "disk file should be removed"); + let row_count: i64 = db + .conn() + .query_row("SELECT COUNT(*) FROM rules WHERE id = ?", [id], |r| { + r.get(0) + }) + .unwrap(); + assert_eq!(row_count, 0, "db row should be removed"); + } + + #[test] + fn test_delete_rule_succeeds_when_file_absent() { + let db = setup_test_db(); + let temp_dir = tempfile::TempDir::new().unwrap(); + + db.conn() + .execute( + "INSERT INTO rules (name, content) VALUES (?, ?)", + params!["ghost", "body"], + ) + .unwrap(); + let id = db.conn().last_insert_rowid(); + + // No disk file created — just the DB row. + let result = delete_rule_inner(db.conn(), id, temp_dir.path()); + assert!( + result.is_ok(), + "delete must succeed when disk file is absent" + ); + + let row_count: i64 = db + .conn() + .query_row("SELECT COUNT(*) FROM rules WHERE id = ?", [id], |r| { + r.get(0) + }) + .unwrap(); + assert_eq!(row_count, 0); + } } diff --git a/src-tauri/src/services/config_writer.rs b/src-tauri/src/services/config_writer.rs index c4811681..00c88a2a 100644 --- a/src-tauri/src/services/config_writer.rs +++ b/src-tauri/src/services/config_writer.rs @@ -697,6 +697,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; let mcps = vec![sample_stdio_mcp()]; @@ -720,6 +721,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; // Write existing config @@ -751,6 +753,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "not valid json").unwrap(); @@ -780,6 +783,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "{}").unwrap(); @@ -815,6 +819,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "{}").unwrap(); @@ -854,6 +859,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "{}").unwrap(); @@ -894,6 +900,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "{}").unwrap(); @@ -933,6 +940,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "{}").unwrap(); @@ -971,6 +979,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; // No file exists, should create new @@ -1096,6 +1105,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; // Create existing project with an MCP @@ -1150,6 +1160,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, r#"{"original": true}"#).unwrap(); @@ -1187,6 +1198,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "{}").unwrap(); @@ -1255,6 +1267,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, r#"{"existing": true}"#).unwrap(); @@ -1340,6 +1353,7 @@ mod tests { commands_dir: dir.path().join("commands"), skills_dir: dir.path().join("skills"), agents_dir: dir.path().join("agents"), + rules_dir: dir.path().join("rules"), }; std::fs::write(&paths.claude_json, "not valid json").unwrap(); diff --git a/src-tauri/src/services/rule_writer.rs b/src-tauri/src/services/rule_writer.rs index 1db3c97d..1582a480 100644 --- a/src-tauri/src/services/rule_writer.rs +++ b/src-tauri/src/services/rule_writer.rs @@ -15,7 +15,16 @@ pub(crate) fn generate_rule_markdown(rule: &Rule) -> String { if let Some(ref paths) = rule.paths { if !paths.is_empty() { - frontmatter.push_str(&format!("paths: {}\n", paths.join(", "))); + frontmatter.push_str(&format!( + "paths: {}\n", + serde_json::to_string(paths).unwrap() + )); + } + } + + if let Some(ref tags) = rule.tags { + if !tags.is_empty() { + frontmatter.push_str(&format!("tags: {}\n", serde_json::to_string(tags).unwrap())); } } @@ -138,16 +147,24 @@ mod tests { } #[test] - fn test_generate_rule_markdown_full() { + fn test_generate_rule_markdown_emits_json_paths() { let rule = sample_rule(); let md = generate_rule_markdown(&rule); assert!(md.starts_with("---\n")); assert!(md.contains("description: Enforce TypeScript strict mode\n")); - assert!(md.contains("paths: src/**/*.ts, tests/**/*.ts\n")); + assert!(md.contains(r#"paths: ["src/**/*.ts","tests/**/*.ts"]"#)); assert!(md.contains("---\n\nAlways use strict TypeScript")); } + #[test] + fn test_generate_rule_markdown_emits_json_tags() { + let rule = sample_rule(); + let md = generate_rule_markdown(&rule); + + assert!(md.contains(r#"tags: ["typescript","quality"]"#)); + } + #[test] fn test_generate_rule_markdown_minimal() { let rule = sample_minimal_rule(); @@ -156,6 +173,7 @@ mod tests { assert!(md.starts_with("---\n")); assert!(!md.contains("description:")); assert!(!md.contains("paths:")); + assert!(!md.contains("tags:")); assert!(md.contains("Be concise.")); } diff --git a/src-tauri/src/services/scanner.rs b/src-tauri/src/services/scanner.rs index 18c59c1a..6b4494bb 100644 --- a/src-tauri/src/services/scanner.rs +++ b/src-tauri/src/services/scanner.rs @@ -19,6 +19,42 @@ use std::path::Path; use tauri::Manager; use walkdir::WalkDir; +/// Source literal used for rows populated by the auto-scan path (as opposed +/// to UI-created rows which carry `source = 'manual'`). +pub(crate) const SOURCE_AUTO_DETECTED: &str = "auto-detected"; + +/// Walk a directory for `*.md` files, invoking `process(&Path)` for each. +/// Returns the count of files for which `process` returned `Ok(true)`. +/// A missing directory is not an error (returns 0). Per-file errors are +/// non-fatal and logged at warn level — scanning continues. +pub(crate) fn walk_md_dir(dir: &Path, mut process: F) -> Result +where + F: FnMut(&Path) -> Result, +{ + let mut count = 0; + if !dir.exists() { + return Ok(0); + } + for entry in std::fs::read_dir(dir)? { + let entry = match entry { + Ok(e) => e, + Err(e) => { + log::warn!("scanner: failed to read dir entry in {:?}: {}", dir, e); + continue; + } + }; + let path = entry.path(); + if path.extension().map(|e| e == "md").unwrap_or(false) { + match process(&path) { + Ok(true) => count += 1, + Ok(false) => {} + Err(e) => log::warn!("scanner: failed to process {}: {}", path.display(), e), + } + } + } + Ok(count) +} + pub async fn run_startup_scan(app: &tauri::AppHandle) -> Result<()> { let db = app.state::>>(); let db = db.lock().map_err(|e| anyhow::anyhow!("{}", e))?; @@ -54,6 +90,10 @@ pub async fn run_startup_scan(app: &tauri::AppHandle) -> Result<()> { let hook_count = scan_global_hooks(&db)?; log::info!("Found {} hooks from ~/.claude/settings.json", hook_count); + // Scan global rules from ~/.claude/rules/ + let rule_count = scan_global_rules(&db)?; + log::info!("Found {} rules from ~/.claude/rules/", rule_count); + // ============================================================================ // OpenCode Scanning // ============================================================================ @@ -247,6 +287,12 @@ pub fn scan_claude_json(db: &Database) -> Result { scan_project_agents(db, project_id, &project_agents_dir)?; } + // Scan project-level rules from .claude/rules/ + let project_rules_dir = Path::new(&path_to_check).join(".claude").join("rules"); + if project_rules_dir.exists() { + scan_project_rules(db, project_id, &project_rules_dir)?; + } + // Scan project-level hooks from .claude/settings.json and .claude/settings.local.json let project_settings_file = Path::new(&path_to_check) .join(".claude") @@ -466,28 +512,14 @@ pub fn scan_plugins(db: &Database) -> Result { /// Scan global commands from ~/.claude/commands/ pub fn scan_global_commands(db: &Database) -> Result { let paths = get_claude_paths()?; - let mut count = 0; - - if paths.commands_dir.exists() { - for entry in std::fs::read_dir(&paths.commands_dir)? { - let entry = entry?; - let path = entry.path(); - - // Only process .md files - if path.extension().map(|e| e == "md").unwrap_or(false) { - if let Some(command) = parse_skill_file(&path) { - // Use get_or_create_command to insert into commands table - let source_path = path.to_string_lossy().to_string(); - let (_, was_created) = get_or_create_command(db, &command, &source_path)?; - if was_created { - count += 1; - } - } - } - } - } - - Ok(count) + walk_md_dir(&paths.commands_dir, |path| { + let Some(command) = parse_skill_file(path) else { + return Ok(false); + }; + let source_path = path.to_string_lossy().to_string(); + let (_, was_created) = get_or_create_command(db, &command, &source_path)?; + Ok(was_created) + }) } /// Scan global skills from ~/.claude/skills/ directories @@ -525,77 +557,145 @@ pub fn scan_global_skills(db: &Database) -> Result { /// Scan global agents from ~/.claude/agents/ pub fn scan_global_agents(db: &Database) -> Result { let paths = get_claude_paths()?; - let mut count = 0; - - if paths.agents_dir.exists() { - for entry in std::fs::read_dir(&paths.agents_dir)? { - let entry = entry?; - let path = entry.path(); + walk_md_dir(&paths.agents_dir, |path| { + let Some(agent) = parse_agent_file(path) else { + return Ok(false); + }; + let source_path = path.to_string_lossy().to_string(); - // Only process .md files - if path.extension().map(|e| e == "md").unwrap_or(false) { - if let Some(agent) = parse_agent_file(&path) { - let source_path = path.to_string_lossy().to_string(); + let existing_id: Option = db + .conn() + .query_row( + "SELECT id FROM subagents WHERE name = ?", + [&agent.name], + |row| row.get(0), + ) + .ok(); - // Check if already exists - let existing_id: Option = db - .conn() - .query_row( - "SELECT id FROM subagents WHERE name = ?", - [&agent.name], - |row| row.get(0), - ) - .ok(); - - if let Some(id) = existing_id { - // Update source_path if not already set - db.conn().execute( - "UPDATE subagents SET source_path = ? WHERE id = ? AND (source_path IS NULL OR source_path = '')", - params![&source_path, id], - )?; - } else { - let tools_json = if agent.tools.is_empty() { - None - } else { - Some(serde_json::to_string(&agent.tools).unwrap()) - }; - let skills_json = if agent.skills.is_empty() { - None - } else { - Some(serde_json::to_string(&agent.skills).unwrap()) - }; - let tags_json = if agent.tags.is_empty() { - None - } else { - Some(serde_json::to_string(&agent.tags).unwrap()) - }; - - let result = db.conn().execute( - "INSERT INTO subagents (name, description, content, tools, model, permission_mode, skills, tags, source, source_path) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, 'auto-detected', ?)", - params![ - agent.name, - agent.description, - agent.content, - tools_json, - agent.model, - agent.permission_mode, - skills_json, - tags_json, - source_path - ], - ); - - if result.is_ok() { - count += 1; - } - } - } - } + if let Some(id) = existing_id { + db.conn().execute( + "UPDATE subagents SET source_path = ? WHERE id = ? AND (source_path IS NULL OR source_path = '')", + params![&source_path, id], + )?; + return Ok(false); } + + let tools_json = + (!agent.tools.is_empty()).then(|| serde_json::to_string(&agent.tools).unwrap()); + let skills_json = + (!agent.skills.is_empty()).then(|| serde_json::to_string(&agent.skills).unwrap()); + let tags_json = + (!agent.tags.is_empty()).then(|| serde_json::to_string(&agent.tags).unwrap()); + + let inserted = db + .conn() + .execute( + "INSERT INTO subagents (name, description, content, tools, model, permission_mode, skills, tags, source, source_path) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + params![ + agent.name, + agent.description, + agent.content, + tools_json, + agent.model, + agent.permission_mode, + skills_json, + tags_json, + SOURCE_AUTO_DETECTED, + source_path + ], + ) + .is_ok(); + Ok(inserted) + }) +} + +/// Scan global rules from ~/.claude/rules/ +pub fn scan_global_rules(db: &Database) -> Result { + let paths = get_claude_paths()?; + walk_md_dir(&paths.rules_dir, |path| { + upsert_rule_from_file(db, path, None) + }) +} + +/// Scan project-level rules from `{project}/.claude/rules/` and assign each +/// rule to the given project. +fn scan_project_rules(db: &Database, project_id: i64, rules_dir: &Path) -> Result { + walk_md_dir(rules_dir, |path| { + upsert_rule_from_file(db, path, Some(project_id)) + }) +} + +/// Parse and upsert a rule file into the `rules` table. Returns `Ok(true)` +/// when a new row was inserted, `Ok(false)` when an existing row was +/// source-path-backfilled. For project-scope scans (`project_id: Some`) a +/// `project_rules` assignment row is also inserted via `INSERT OR IGNORE`. +fn upsert_rule_from_file(db: &Database, path: &Path, project_id: Option) -> Result { + let Some(parsed) = parse_rule_file(path) else { + return Ok(false); + }; + let source_path = path.to_string_lossy().to_string(); + + let symlink_meta = std::fs::symlink_metadata(path).ok(); + let is_symlink = symlink_meta + .as_ref() + .map(|m| m.file_type().is_symlink()) + .unwrap_or(false); + let symlink_target = if is_symlink { + std::fs::read_link(path) + .ok() + .map(|p| p.to_string_lossy().to_string()) + } else { + None + }; + + let existing_id: Option = db + .conn() + .query_row( + "SELECT id FROM rules WHERE name = ?", + [&parsed.name], + |row| row.get(0), + ) + .ok(); + + let (rule_id, inserted) = if let Some(id) = existing_id { + db.conn().execute( + "UPDATE rules SET source_path = ? WHERE id = ? AND (source_path IS NULL OR source_path = '')", + params![&source_path, id], + )?; + (id, false) + } else { + let paths_json = + (!parsed.paths.is_empty()).then(|| serde_json::to_string(&parsed.paths).unwrap()); + let tags_json = + (!parsed.tags.is_empty()).then(|| serde_json::to_string(&parsed.tags).unwrap()); + + db.conn().execute( + "INSERT INTO rules (name, description, content, paths, tags, source, source_path, is_symlink, symlink_target) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)", + params![ + parsed.name, + parsed.description, + parsed.content, + paths_json, + tags_json, + SOURCE_AUTO_DETECTED, + source_path, + is_symlink as i32, + symlink_target, + ], + )?; + (db.conn().last_insert_rowid(), true) + }; + + if let Some(pid) = project_id { + db.conn().execute( + "INSERT OR IGNORE INTO project_rules (project_id, rule_id) VALUES (?, ?)", + params![pid, rule_id], + )?; } - Ok(count) + Ok(inserted) } /// Parsed skill data from markdown file @@ -633,6 +733,57 @@ pub(crate) struct ParsedAgent { pub(crate) tags: Vec, } +/// Parsed rule data from a `.claude/rules/.md` file. +#[derive(Debug, PartialEq)] +pub(crate) struct ParsedRule { + pub(crate) name: String, + pub(crate) description: Option, + pub(crate) paths: Vec, + pub(crate) tags: Vec, + pub(crate) content: String, +} + +/// Parse a rule markdown file. Returns `None` if the file cannot be read +/// or the stem is not a valid UTF-8 name. +pub(crate) fn parse_rule_file(path: &Path) -> Option { + let name = path.file_stem()?.to_string_lossy().to_string(); + let content = std::fs::read_to_string(path).ok()?; + let (frontmatter, body) = parse_frontmatter(&content); + let description = frontmatter.get("description").cloned(); + let paths = frontmatter + .get("paths") + .map(|v| parse_list_value(v)) + .unwrap_or_default(); + let tags = frontmatter + .get("tags") + .map(|v| parse_list_value(v)) + .unwrap_or_default(); + Some(ParsedRule { + name, + description, + paths, + tags, + content: body, + }) +} + +/// Parse a `paths:` or `tags:` frontmatter value. Accepts both the JSON +/// array form (`["a","b"]`) emitted by the current writer and the legacy +/// comma-separated form (`a, b`) written by earlier versions. +pub(crate) fn parse_list_value(raw: &str) -> Vec { + let trimmed = raw.trim(); + if trimmed.starts_with('[') { + if let Ok(v) = serde_json::from_str::>(trimmed) { + return v; + } + } + trimmed + .split(',') + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + .collect() +} + /// Parse a skill markdown file pub(crate) fn parse_skill_file(path: &Path) -> Option { let content = std::fs::read_to_string(path).ok()?; @@ -872,28 +1023,15 @@ pub(crate) fn parse_frontmatter( /// Scan project-level commands from .claude/commands/ and assign to project fn scan_project_commands(db: &Database, project_id: i64, commands_dir: &Path) -> Result { - let mut count = 0; - - for entry in std::fs::read_dir(commands_dir)? { - let entry = entry?; - let path = entry.path(); - - // Only process .md files - if path.extension().map(|e| e == "md").unwrap_or(false) { - if let Some(command) = parse_skill_file(&path) { - // Get or create the command in the library - let source_path = path.to_string_lossy().to_string(); - let (command_id, _) = get_or_create_command(db, &command, &source_path)?; - - // Assign command to project if not already assigned - assign_command_to_project(db, project_id, command_id)?; - - count += 1; - } - } - } - - Ok(count) + walk_md_dir(commands_dir, |path| { + let Some(command) = parse_skill_file(path) else { + return Ok(false); + }; + let source_path = path.to_string_lossy().to_string(); + let (command_id, _) = get_or_create_command(db, &command, &source_path)?; + assign_command_to_project(db, project_id, command_id)?; + Ok(true) + }) } /// Scan project-level skills from .claude/skills/ and assign to project @@ -929,28 +1067,15 @@ fn scan_project_skills(db: &Database, project_id: i64, skills_dir: &Path) -> Res /// Scan project-level agents and assign to project fn scan_project_agents(db: &Database, project_id: i64, agents_dir: &Path) -> Result { - let mut count = 0; - - for entry in std::fs::read_dir(agents_dir)? { - let entry = entry?; - let path = entry.path(); - - // Only process .md files - if path.extension().map(|e| e == "md").unwrap_or(false) { - if let Some(agent) = parse_agent_file(&path) { - // Get or create the agent in the library - let source_path = path.to_string_lossy().to_string(); - let agent_id = get_or_create_agent(db, &agent, &source_path)?; - - // Assign agent to project if not already assigned - assign_agent_to_project(db, project_id, agent_id)?; - - count += 1; - } - } - } - - Ok(count) + walk_md_dir(agents_dir, |path| { + let Some(agent) = parse_agent_file(path) else { + return Ok(false); + }; + let source_path = path.to_string_lossy().to_string(); + let agent_id = get_or_create_agent(db, &agent, &source_path)?; + assign_agent_to_project(db, project_id, agent_id)?; + Ok(true) + }) } /// Get or create a skill in the database, returning (skill_id, was_created) @@ -4014,4 +4139,324 @@ Body content."#, .unwrap(); assert_eq!(assigned, 1); } + + // ========================================================================= + // walk_md_dir + rule scanner tests + // ========================================================================= + + #[test] + fn test_walk_md_dir_empty_when_missing() { + let temp_dir = TempDir::new().unwrap(); + let missing = temp_dir.path().join("does-not-exist"); + let mut called = 0; + let count = walk_md_dir(&missing, |_| { + called += 1; + Ok(true) + }) + .unwrap(); + assert_eq!(count, 0); + assert_eq!(called, 0); + } + + #[test] + fn test_walk_md_dir_skips_non_md() { + let temp_dir = TempDir::new().unwrap(); + fs::write(temp_dir.path().join("a.md"), "x").unwrap(); + fs::write(temp_dir.path().join("b.txt"), "x").unwrap(); + fs::write(temp_dir.path().join("c.json"), "x").unwrap(); + fs::write(temp_dir.path().join("d.md"), "x").unwrap(); + + let mut seen = Vec::new(); + let count = walk_md_dir(temp_dir.path(), |p| { + seen.push(p.file_name().unwrap().to_string_lossy().to_string()); + Ok(true) + }) + .unwrap(); + assert_eq!(count, 2); + seen.sort(); + assert_eq!(seen, vec!["a.md".to_string(), "d.md".to_string()]); + } + + #[test] + fn test_walk_md_dir_continues_on_per_file_error() { + let temp_dir = TempDir::new().unwrap(); + fs::write(temp_dir.path().join("good.md"), "x").unwrap(); + fs::write(temp_dir.path().join("bad.md"), "x").unwrap(); + + let count = walk_md_dir(temp_dir.path(), |p| { + if p.file_name().unwrap() == "bad.md" { + Err(anyhow::anyhow!("simulated failure")) + } else { + Ok(true) + } + }) + .unwrap(); + assert_eq!(count, 1); + } + + #[test] + fn test_parse_rule_file_full_frontmatter() { + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir.path().join("ts-strict.md"); + fs::write( + &path, + r#"--- +description: Enforce TS strict mode +paths: ["src/**/*.ts", "tests/**/*.ts"] +tags: ["typescript", "quality"] +--- +Body content here."#, + ) + .unwrap(); + + let parsed = parse_rule_file(&path).unwrap(); + assert_eq!(parsed.name, "ts-strict"); + assert_eq!( + parsed.description.as_deref(), + Some("Enforce TS strict mode") + ); + assert_eq!( + parsed.paths, + vec!["src/**/*.ts".to_string(), "tests/**/*.ts".to_string()] + ); + assert_eq!( + parsed.tags, + vec!["typescript".to_string(), "quality".to_string()] + ); + assert_eq!(parsed.content, "Body content here."); + } + + #[test] + fn test_parse_rule_file_no_frontmatter() { + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir.path().join("plain.md"); + fs::write(&path, "Just body, no frontmatter.").unwrap(); + + let parsed = parse_rule_file(&path).unwrap(); + assert_eq!(parsed.name, "plain"); + assert!(parsed.description.is_none()); + assert!(parsed.paths.is_empty()); + assert!(parsed.tags.is_empty()); + assert_eq!(parsed.content, "Just body, no frontmatter."); + } + + #[test] + fn test_parse_rule_file_invalid_markdown() { + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir.path().join("weird.md"); + fs::write(&path, "--- not a real frontmatter block").unwrap(); + + let parsed = parse_rule_file(&path).unwrap(); + assert_eq!(parsed.name, "weird"); + assert!(parsed.description.is_none()); + assert!(parsed.paths.is_empty()); + } + + #[test] + fn test_parse_list_value_json_form() { + assert_eq!( + parse_list_value(r#"["a", "b", "c"]"#), + vec!["a".to_string(), "b".to_string(), "c".to_string()] + ); + } + + #[test] + fn test_parse_list_value_comma_form() { + assert_eq!( + parse_list_value("a, b, c"), + vec!["a".to_string(), "b".to_string(), "c".to_string()] + ); + } + + #[test] + fn test_parse_list_value_empty() { + assert!(parse_list_value("").is_empty()); + assert!(parse_list_value(" ").is_empty()); + assert!(parse_list_value(", ,").is_empty()); + } + + fn scan_global_rules_in(db: &Database, rules_dir: &Path) -> Result { + walk_md_dir(rules_dir, |path| upsert_rule_from_file(db, path, None)) + } + + #[test] + fn test_scan_global_rules_empty_dir() { + let db = setup_test_db(); + let temp_dir = TempDir::new().unwrap(); + let count = scan_global_rules_in(&db, temp_dir.path()).unwrap(); + assert_eq!(count, 0); + } + + #[test] + fn test_scan_global_rules_inserts_new() { + let db = setup_test_db(); + let temp_dir = TempDir::new().unwrap(); + fs::write( + temp_dir.path().join("r1.md"), + r#"--- +description: Rule one +paths: ["src/**/*.ts"] +--- +R1 body"#, + ) + .unwrap(); + fs::write(temp_dir.path().join("r2.md"), "R2 body, no frontmatter").unwrap(); + + let count = scan_global_rules_in(&db, temp_dir.path()).unwrap(); + assert_eq!(count, 2); + + let row_count: i64 = db + .conn() + .query_row("SELECT COUNT(*) FROM rules", [], |r| r.get(0)) + .unwrap(); + assert_eq!(row_count, 2); + + let (paths_json, source): (Option, String) = db + .conn() + .query_row( + "SELECT paths, source FROM rules WHERE name = ?", + ["r1"], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .unwrap(); + assert_eq!(source, SOURCE_AUTO_DETECTED); + let parsed: Vec = serde_json::from_str(&paths_json.unwrap()).unwrap(); + assert_eq!(parsed, vec!["src/**/*.ts".to_string()]); + } + + #[test] + fn test_scan_global_rules_idempotent_rescan() { + let db = setup_test_db(); + let temp_dir = TempDir::new().unwrap(); + fs::write(temp_dir.path().join("r.md"), "Body").unwrap(); + + assert_eq!(scan_global_rules_in(&db, temp_dir.path()).unwrap(), 1); + // Second call returns 0 (no new inserts), no duplicates. + assert_eq!(scan_global_rules_in(&db, temp_dir.path()).unwrap(), 0); + let row_count: i64 = db + .conn() + .query_row("SELECT COUNT(*) FROM rules", [], |r| r.get(0)) + .unwrap(); + assert_eq!(row_count, 1); + } + + #[test] + fn test_scan_global_rules_backfills_source_path() { + let db = setup_test_db(); + // Insert a UI-created rule with NULL source_path first. + db.conn() + .execute( + "INSERT INTO rules (name, content, source) VALUES (?, ?, 'manual')", + params!["pre-existing", "old body"], + ) + .unwrap(); + + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("pre-existing.md"); + fs::write(&file_path, "new body").unwrap(); + + let count = scan_global_rules_in(&db, temp_dir.path()).unwrap(); + // No new row — existing row was backfilled. + assert_eq!(count, 0); + + let (source_path, source): (Option, String) = db + .conn() + .query_row( + "SELECT source_path, source FROM rules WHERE name = ?", + ["pre-existing"], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .unwrap(); + assert_eq!( + source_path.as_deref(), + Some(file_path.to_string_lossy().as_ref()) + ); + // Source is NOT overwritten — manual stays manual. + assert_eq!(source, "manual"); + } + + #[cfg(unix)] + #[test] + fn test_scan_global_rules_populates_symlink_fields() { + let db = setup_test_db(); + let temp_dir = TempDir::new().unwrap(); + + let target = temp_dir.path().join("target.md"); + fs::write(&target, "target body").unwrap(); + let link = temp_dir.path().join("linked.md"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + let count = scan_global_rules_in(&db, temp_dir.path()).unwrap(); + assert_eq!(count, 2); + + let (is_symlink, sym_target): (i32, Option) = db + .conn() + .query_row( + "SELECT is_symlink, symlink_target FROM rules WHERE name = ?", + ["linked"], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .unwrap(); + assert_eq!(is_symlink, 1); + assert_eq!( + sym_target.as_deref(), + Some(target.to_string_lossy().as_ref()) + ); + + let (is_symlink_t, _): (i32, Option) = db + .conn() + .query_row( + "SELECT is_symlink, symlink_target FROM rules WHERE name = ?", + ["target"], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .unwrap(); + assert_eq!(is_symlink_t, 0); + } + + #[test] + fn test_scan_project_rules_inserts_assignment() { + let db = setup_test_db(); + let proj_id = get_or_create_project(&db, "proj", "/tmp/proj").unwrap(); + let temp_dir = TempDir::new().unwrap(); + fs::write( + temp_dir.path().join("r.md"), + r#"--- +description: Project rule +--- +Body"#, + ) + .unwrap(); + + let count = scan_project_rules(&db, proj_id, temp_dir.path()).unwrap(); + assert_eq!(count, 1); + + let rule_row_count: i64 = db + .conn() + .query_row("SELECT COUNT(*) FROM rules", [], |r| r.get(0)) + .unwrap(); + assert_eq!(rule_row_count, 1); + + let assigned: i64 = db + .conn() + .query_row( + "SELECT COUNT(*) FROM project_rules WHERE project_id = ?", + [proj_id], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(assigned, 1); + + // Rescanning does not duplicate the assignment. + scan_project_rules(&db, proj_id, temp_dir.path()).unwrap(); + let assigned2: i64 = db + .conn() + .query_row( + "SELECT COUNT(*) FROM project_rules WHERE project_id = ?", + [proj_id], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(assigned2, 1); + } } diff --git a/src-tauri/src/utils/paths.rs b/src-tauri/src/utils/paths.rs index b4084a79..31d6bd20 100644 --- a/src-tauri/src/utils/paths.rs +++ b/src-tauri/src/utils/paths.rs @@ -13,6 +13,7 @@ pub struct ClaudePathsInternal { pub commands_dir: PathBuf, // ~/.claude/commands/ for command-type skills pub skills_dir: PathBuf, // ~/.claude/skills/ for agent-type skills pub agents_dir: PathBuf, // ~/.claude/agents/ for sub-agents + pub rules_dir: PathBuf, // ~/.claude/rules/ for behavioral rules } pub fn get_claude_paths() -> Result { @@ -30,6 +31,7 @@ pub fn get_claude_paths() -> Result { commands_dir: claude_dir.join("commands"), skills_dir: claude_dir.join("skills"), agents_dir: claude_dir.join("agents"), + rules_dir: claude_dir.join("rules"), home, claude_dir, }) @@ -66,6 +68,7 @@ mod tests { assert!(paths.commands_dir.ends_with("commands")); assert!(paths.skills_dir.ends_with("skills")); assert!(paths.agents_dir.ends_with("agents")); + assert!(paths.rules_dir.ends_with("rules")); } #[test] From 1c9f2a4c71245616bb8fbe0b57fda0e71ea10b9a Mon Sep 17 00:00:00 2001 From: Scot Campbell Date: Tue, 21 Apr 2026 11:42:59 -0400 Subject: [PATCH 2/2] fix: surface real DB errors in delete_rule_inner instead of swallowing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review nit on #200. The previous `.ok().and_then(|s| s.query_row(..).ok())` pattern silently conflated three cases: 1. row exists → Some(rule) ✓ 2. row absent (QueryReturnedNoRows) → None ✓ 3. real DB error (lock contention, corruption, IO) → None ✗ → DB delete proceeds, disk file leaks Switch to an explicit match on rusqlite::Error so only QueryReturnedNoRows maps to None; every other error is returned before the DELETE runs. Same treatment for rule_writer::delete_rule_file — its existing file-exists guard already handles the legitimate "disk file already gone" case, so any error it raises now is a real IO/permission failure worth surfacing. Adds test_delete_rule_missing_id_is_not_an_error to pin the QueryReturnedNoRows arm. 2015/2015 lib tests pass. --- src-tauri/src/commands/rules.rs | 38 ++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/commands/rules.rs b/src-tauri/src/commands/rules.rs index 7510f583..ab0e25fb 100644 --- a/src-tauri/src/commands/rules.rs +++ b/src-tauri/src/commands/rules.rs @@ -139,23 +139,33 @@ pub fn update_rule( stmt.query_row([id], row_to_rule).map_err(|e| e.to_string()) } -/// Inner helper for [`delete_rule`]: drops the DB row and best-effort -/// removes the backing `.md` file under `{home}/.claude/rules/.md`. +/// Inner helper for [`delete_rule`]: drops the DB row and removes the +/// backing `.md` file under `{home}/.claude/rules/.md`. /// Factored out so tests can pass a temp dir in place of `~`. +/// +/// Error handling: a missing row is not an error (the rule is effectively +/// gone — DB delete no-ops, disk delete is skipped). Any other lookup error +/// (lock contention, corruption, etc.) is surfaced *before* the DB delete, +/// so we never orphan the disk file by nulling out the DB row without +/// knowing the filename. fn delete_rule_inner(conn: &rusqlite::Connection, id: i64, home: &Path) -> Result<(), String> { let query = format!("SELECT {} FROM rules WHERE id = ?", RULE_SELECT_FIELDS); - let rule: Option = conn + let rule: Option = match conn .prepare(&query) - .ok() - .and_then(|mut s| s.query_row([id], row_to_rule).ok()); + .and_then(|mut s| s.query_row([id], row_to_rule)) + { + Ok(r) => Some(r), + Err(rusqlite::Error::QueryReturnedNoRows) => None, + Err(e) => return Err(e.to_string()), + }; conn.execute("DELETE FROM rules WHERE id = ?", [id]) .map_err(|e| e.to_string())?; if let Some(rule) = rule { - // Best-effort — if the file is already gone (user deleted by hand) we - // still want the DB delete to succeed. - let _ = rule_writer::delete_rule_file(home, &rule); + // `delete_rule_file` already treats a missing file as success; any + // error returned here is a real IO/permission failure worth surfacing. + rule_writer::delete_rule_file(home, &rule).map_err(|e| e.to_string())?; } Ok(()) @@ -611,6 +621,18 @@ mod tests { assert_eq!(row_count, 0, "db row should be removed"); } + #[test] + fn test_delete_rule_missing_id_is_not_an_error() { + // A delete targeting an ID that isn't in the DB must return Ok — + // the "rule is gone" end state is already satisfied. This exercises + // the `QueryReturnedNoRows` arm of delete_rule_inner. + let db = setup_test_db(); + let temp_dir = tempfile::TempDir::new().unwrap(); + + let result = delete_rule_inner(db.conn(), 99_999, temp_dir.path()); + assert!(result.is_ok(), "missing row should not error"); + } + #[test] fn test_delete_rule_succeeds_when_file_absent() { let db = setup_test_db();