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:
@@ -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"],
|
||||||
|
|||||||
@@ -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.
|
||||||
|
|||||||
@@ -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);
|
||||||
|
|||||||
@@ -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.
|
||||||
|
|||||||
Reference in New Issue
Block a user