Skip to content

Commit 1c9f2a4

Browse files
committed
fix: surface real DB errors in delete_rule_inner instead of swallowing them
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.
1 parent 310524d commit 1c9f2a4

1 file changed

Lines changed: 30 additions & 8 deletions

File tree

src-tauri/src/commands/rules.rs

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -139,23 +139,33 @@ pub fn update_rule(
139139
stmt.query_row([id], row_to_rule).map_err(|e| e.to_string())
140140
}
141141

142-
/// Inner helper for [`delete_rule`]: drops the DB row and best-effort
143-
/// removes the backing `.md` file under `{home}/.claude/rules/<name>.md`.
142+
/// Inner helper for [`delete_rule`]: drops the DB row and removes the
143+
/// backing `.md` file under `{home}/.claude/rules/<name>.md`.
144144
/// Factored out so tests can pass a temp dir in place of `~`.
145+
///
146+
/// Error handling: a missing row is not an error (the rule is effectively
147+
/// gone — DB delete no-ops, disk delete is skipped). Any other lookup error
148+
/// (lock contention, corruption, etc.) is surfaced *before* the DB delete,
149+
/// so we never orphan the disk file by nulling out the DB row without
150+
/// knowing the filename.
145151
fn delete_rule_inner(conn: &rusqlite::Connection, id: i64, home: &Path) -> Result<(), String> {
146152
let query = format!("SELECT {} FROM rules WHERE id = ?", RULE_SELECT_FIELDS);
147-
let rule: Option<Rule> = conn
153+
let rule: Option<Rule> = match conn
148154
.prepare(&query)
149-
.ok()
150-
.and_then(|mut s| s.query_row([id], row_to_rule).ok());
155+
.and_then(|mut s| s.query_row([id], row_to_rule))
156+
{
157+
Ok(r) => Some(r),
158+
Err(rusqlite::Error::QueryReturnedNoRows) => None,
159+
Err(e) => return Err(e.to_string()),
160+
};
151161

152162
conn.execute("DELETE FROM rules WHERE id = ?", [id])
153163
.map_err(|e| e.to_string())?;
154164

155165
if let Some(rule) = rule {
156-
// Best-effort — if the file is already gone (user deleted by hand) we
157-
// still want the DB delete to succeed.
158-
let _ = rule_writer::delete_rule_file(home, &rule);
166+
// `delete_rule_file` already treats a missing file as success; any
167+
// error returned here is a real IO/permission failure worth surfacing.
168+
rule_writer::delete_rule_file(home, &rule).map_err(|e| e.to_string())?;
159169
}
160170

161171
Ok(())
@@ -611,6 +621,18 @@ mod tests {
611621
assert_eq!(row_count, 0, "db row should be removed");
612622
}
613623

624+
#[test]
625+
fn test_delete_rule_missing_id_is_not_an_error() {
626+
// A delete targeting an ID that isn't in the DB must return Ok —
627+
// the "rule is gone" end state is already satisfied. This exercises
628+
// the `QueryReturnedNoRows` arm of delete_rule_inner.
629+
let db = setup_test_db();
630+
let temp_dir = tempfile::TempDir::new().unwrap();
631+
632+
let result = delete_rule_inner(db.conn(), 99_999, temp_dir.path());
633+
assert!(result.is_ok(), "missing row should not error");
634+
}
635+
614636
#[test]
615637
fn test_delete_rule_succeeds_when_file_absent() {
616638
let db = setup_test_db();

0 commit comments

Comments
 (0)