refactor(cli): apply simplify findings (Plan B Phases 4-6 polish)

- session.rs: drop save_manifest_raw — its only caller was
  after_manifest_change itself; the pub(crate) advertised the exact
  bypass-the-cache-refresh footgun the wrapper exists to eliminate.
  Inline the encrypt + atomic_write pair.
- session.rs: into_kdf_params(self) → to_kdf_params(&self). Body just
  copies three u32s; the consume-self had no ownership benefit and
  forced the round-trip test to rebuild a ParamsFile field-by-field.
- helpers.rs: add git_rm(repo, paths, context) wrapper around git_run
  + the load-bearing --ignore-unmatch flag. Replaces two near-identical
  three-line "build rm_args, extend, git_run" blocks in trash.rs.
- trash.rs: purge_item_filesystem drops the if x.exists() pre-checks
  (TOCTOU + redundant stat per item per trash-empty iteration). Uses
  ErrorKind::NotFound swallow on remove_file/remove_dir_all instead.
- basic_flows.rs: trim trash_empty_batches_into_one_commit's sleep
  comment to just the WHY.
This commit is contained in:
adlee-was-taken
2026-05-09 11:50:42 -04:00
parent 4b657e71f1
commit c4777cc0bb
4 changed files with 29 additions and 38 deletions

View File

@@ -49,15 +49,20 @@ pub(super) fn purge_item_filesystem(
id: &relicario_core::ItemId, id: &relicario_core::ItemId,
title: &str, title: &str,
) -> Result<Vec<String>> { ) -> Result<Vec<String>> {
use std::fs; use std::{fs, io::ErrorKind};
let item_rel = format!("items/{}.enc", id.as_str()); let item_rel = format!("items/{}.enc", id.as_str());
let att_rel = format!("attachments/{}", id.as_str()); let att_rel = format!("attachments/{}", id.as_str());
let item_path = vault.item_path(id); let ignore_missing = |r: std::io::Result<()>| -> Result<()> {
if item_path.exists() { fs::remove_file(&item_path)?; } match r {
let att_dir = vault.root().join("attachments").join(id.as_str()); Ok(()) => Ok(()),
if att_dir.exists() { fs::remove_dir_all(&att_dir)?; } Err(e) if e.kind() == ErrorKind::NotFound => Ok(()),
Err(e) => Err(e.into()),
}
};
ignore_missing(fs::remove_file(vault.item_path(id)))?;
ignore_missing(fs::remove_dir_all(vault.root().join("attachments").join(id.as_str())))?;
manifest.remove(id); manifest.remove(id);
eprintln!("Purged: {title}"); eprintln!("Purged: {title}");
@@ -76,10 +81,7 @@ pub fn cmd_purge(query: String) -> Result<()> {
vault.after_manifest_change(&manifest)?; vault.after_manifest_change(&manifest)?;
let purge_ctx = format!("purge \"{}\" ({})", title, id.as_str()); let purge_ctx = format!("purge \"{}\" ({})", title, id.as_str());
let mut rm_args: Vec<&str> = vec!["rm", "-rf", "--ignore-unmatch"]; crate::helpers::git_rm(vault.root(), &paths, &format!("{purge_ctx}: git rm"))?;
let path_refs: Vec<&str> = paths.iter().map(String::as_str).collect();
rm_args.extend(path_refs.iter().copied());
crate::helpers::git_run(vault.root(), &rm_args, &format!("{purge_ctx}: git rm"))?;
crate::helpers::git_run( crate::helpers::git_run(
vault.root(), vault.root(),
&["add", "manifest.enc"], &["add", "manifest.enc"],
@@ -130,10 +132,7 @@ pub fn cmd_trash_empty() -> Result<()> {
vault.after_manifest_change(&manifest)?; vault.after_manifest_change(&manifest)?;
let mut rm_args: Vec<&str> = vec!["rm", "-rf", "--ignore-unmatch"]; crate::helpers::git_rm(vault.root(), &all_paths, "trash empty: git rm")?;
let path_refs: Vec<&str> = all_paths.iter().map(String::as_str).collect();
rm_args.extend(path_refs.iter().copied());
crate::helpers::git_run(vault.root(), &rm_args, "trash empty: git rm")?;
crate::helpers::git_run( crate::helpers::git_run(
vault.root(), vault.root(),
&["add", "manifest.enc"], &["add", "manifest.enc"],

View File

@@ -86,6 +86,16 @@ pub fn git_run(repo: &Path, args: &[&str], context: &str) -> Result<()> {
Ok(()) Ok(())
} }
/// Stage `paths` for removal in one `git rm -rf --ignore-unmatch` invocation.
/// `--ignore-unmatch` is load-bearing: a previous partial-write crash can
/// leave the manifest entry without the corresponding `items/<id>.enc` on
/// disk, and we want the rm to succeed regardless.
pub fn git_rm(repo: &Path, paths: &[String], context: &str) -> Result<()> {
let mut args: Vec<&str> = vec!["rm", "-rf", "--ignore-unmatch"];
args.extend(paths.iter().map(String::as_str));
git_run(repo, &args, context)
}
/// Format a Unix-seconds timestamp as an ISO-8601 UTC string. /// Format a Unix-seconds timestamp as an ISO-8601 UTC string.
/// Audit M11: replaces the old `now_iso8601` helper that actually returned /// Audit M11: replaces the old `now_iso8601` helper that actually returned
/// a numeric string. /// a numeric string.

View File

@@ -74,19 +74,12 @@ impl UnlockedVault {
/// changes the manifest goes through this method, so cache freshness is /// changes the manifest goes through this method, so cache freshness is
/// a compile-time invariant rather than a discipline rule. /// a compile-time invariant rather than a discipline rule.
pub fn after_manifest_change(&self, manifest: &Manifest) -> Result<()> { pub fn after_manifest_change(&self, manifest: &Manifest) -> Result<()> {
self.save_manifest_raw(manifest)?; let bytes = encrypt_manifest(manifest, &self.master_key)?;
atomic_write(&self.manifest_path(), &bytes)?;
crate::helpers::refresh_groups_cache(&self.root, manifest); crate::helpers::refresh_groups_cache(&self.root, manifest);
Ok(()) Ok(())
} }
/// Encrypt the manifest and atomically write it. Most callers want
/// `after_manifest_change` instead — this method skips the groups.cache
/// refresh, leaving shell completion stale until the next mutation.
pub(crate) fn save_manifest_raw(&self, manifest: &Manifest) -> Result<()> {
let bytes = encrypt_manifest(manifest, &self.master_key)?;
atomic_write(&self.manifest_path(), &bytes)
}
pub fn load_settings(&self) -> Result<VaultSettings> { pub fn load_settings(&self) -> Result<VaultSettings> {
let bytes = fs::read(self.settings_path()).context("failed to read settings.enc")?; let bytes = fs::read(self.settings_path()).context("failed to read settings.enc")?;
Ok(decrypt_settings(&bytes, &self.master_key)?) Ok(decrypt_settings(&bytes, &self.master_key)?)
@@ -152,7 +145,7 @@ impl ParamsFile {
} }
} }
pub fn into_kdf_params(self) -> KdfParams { pub fn to_kdf_params(&self) -> KdfParams {
KdfParams { KdfParams {
argon2_m: self.kdf.argon2_m, argon2_m: self.kdf.argon2_m,
argon2_t: self.kdf.argon2_t, argon2_t: self.kdf.argon2_t,
@@ -165,7 +158,7 @@ fn read_params(root: &Path) -> Result<KdfParams> {
let s = fs::read_to_string(root.join(".relicario").join("params.json")) let s = fs::read_to_string(root.join(".relicario").join("params.json"))
.context("failed to read .relicario/params.json")?; .context("failed to read .relicario/params.json")?;
let pf: ParamsFile = serde_json::from_str(&s).context("failed to parse params.json")?; let pf: ParamsFile = serde_json::from_str(&s).context("failed to parse params.json")?;
Ok(pf.into_kdf_params()) Ok(pf.to_kdf_params())
} }
/// Locate the reference image path via `RELICARIO_IMAGE` env var or interactive prompt. /// Locate the reference image path via `RELICARIO_IMAGE` env var or interactive prompt.
@@ -225,18 +218,7 @@ mod tests {
assert_eq!(pf.aead, "xchacha20poly1305"); assert_eq!(pf.aead, "xchacha20poly1305");
assert_eq!(pf.salt_path, ".relicario/salt"); assert_eq!(pf.salt_path, ".relicario/salt");
let kdf = ParamsFile { let kdf = pf.to_kdf_params();
format_version: pf.format_version,
kdf: ParamsKdf {
algorithm: pf.kdf.algorithm.clone(),
argon2_m: pf.kdf.argon2_m,
argon2_t: pf.kdf.argon2_t,
argon2_p: pf.kdf.argon2_p,
},
aead: pf.aead.clone(),
salt_path: pf.salt_path.clone(),
}
.into_kdf_params();
assert_eq!(kdf.argon2_m, 65536); assert_eq!(kdf.argon2_m, 65536);
assert_eq!(kdf.argon2_t, 3); assert_eq!(kdf.argon2_t, 3);
assert_eq!(kdf.argon2_p, 4); assert_eq!(kdf.argon2_p, 4);

View File

@@ -135,8 +135,8 @@ fn trash_empty_batches_into_one_commit() {
let out = v.run(&["settings", "trash-retention", "--days", "0"]); let out = v.run(&["settings", "trash-retention", "--days", "0"]);
assert!(out.status.success(), "settings trash-retention failed"); assert!(out.status.success(), "settings trash-retention failed");
// Brief sleep to ensure now > trashed_at by at least 1 second // should_purge uses strict > on (now - trashed_at), so equal-second
// (otherwise should_purge returns false at strict-greater-than equality). // timestamps don't qualify.
std::thread::sleep(std::time::Duration::from_secs(1)); std::thread::sleep(std::time::Duration::from_secs(1));
// Count commits before. // Count commits before.