diff --git a/app/src-tauri/src/commands/file_commands.rs b/app/src-tauri/src/commands/file_commands.rs index a301e40..99c948b 100644 --- a/app/src-tauri/src/commands/file_commands.rs +++ b/app/src-tauri/src/commands/file_commands.rs @@ -978,7 +978,7 @@ pub async fn resolve_host_read_path(path: &str) -> Result { /// `Path::file_name` so a Windows path is split as one wherever this runs, and /// the answer goes through [`validate_entry_name`] because it becomes a tar /// entry name, a container path and an argv element. -fn host_upload_name(path: &str) -> Result { +pub(crate) fn host_upload_name(path: &str) -> Result { if normalize_host_path(path).ends_with('/') { // A trailing separator names a directory, and `Downloads` is not the // name of a file to upload. The recursive-upload refusal further down diff --git a/app/src-tauri/src/commands/terminal_commands.rs b/app/src-tauri/src/commands/terminal_commands.rs index 799b479..c4d1202 100644 --- a/app/src-tauri/src/commands/terminal_commands.rs +++ b/app/src-tauri/src/commands/terminal_commands.rs @@ -203,6 +203,13 @@ pub async fn upload_host_file_to_terminal( // symlinks already resolved, so a visible directory that *leads* to `~/.ssh` // is refused too. What comes back is that resolved path, and it is what // gets opened. + // The name is taken from the path the user actually dropped, *before* + // resolution. Deriving it from the resolved path renames the file behind + // the user's back: dropping `~/Downloads/latest.log`, where `latest.log` is + // a symlink, would land it in the container as `2026-08-23.log`. The Files + // pane's upload had the same bug and fixes it the same way — one helper, so + // the two drop targets cannot drift. + let base = crate::commands::file_commands::host_upload_name(&host_path)?; let host_path = crate::commands::file_commands::resolve_host_read_path(&host_path).await?; let container_id = state.exec_manager.get_container_id(&session_id).await?; @@ -228,11 +235,7 @@ pub async fn upload_host_file_to_terminal( )); } - let base = std::path::Path::new(&host_path) - .file_name() - .map(|s| s.to_string_lossy().to_string()) - .filter(|s| !s.is_empty()) - .unwrap_or_else(|| "dropped-file".to_string()); + // Ensure the destination directory exists rather than relying on Docker's // archive extractor to create the parent for the uploaded tar entry. @@ -295,3 +298,38 @@ pub async fn stop_audio_bridge( state.exec_manager.close_session(&audio_session_id).await; Ok(()) } + +#[cfg(test)] +mod tests { + /// Both drop targets must name a dropped file the way the *user* named it. + /// + /// The bug this pins: `upload_host_file_to_terminal` derived the tar entry + /// name from the path *after* symlink resolution, so dropping + /// `~/Downloads/latest.log` — where `latest.log` is a symlink to + /// `2026-08-23.log` — silently landed the file in the container under the + /// target's name. Nothing errored; the user just got a name they never + /// typed. The Files pane had the identical bug. + /// + /// What actually keeps the two from drifting is that they now call one + /// helper, so this asserts that helper's contract from the terminal side: + /// the answer comes from the spelling, and a path that does not name a file + /// is refused rather than silently substituted (it used to fall back to + /// `"dropped-file"`). + #[test] + fn a_dropped_file_keeps_the_name_the_user_dropped() { + use crate::commands::file_commands::host_upload_name; + + assert_eq!( + host_upload_name("/home/u/Downloads/latest.log").unwrap(), + "latest.log" + ); + assert!( + host_upload_name("/home/u/Downloads/").is_err(), + "a directory is not a file to drop" + ); + assert!( + host_upload_name("/home/u/..").is_err(), + "the name becomes a tar entry, a container path and an argv element" + ); + } +} diff --git a/app/src-tauri/src/storage/migration_store.rs b/app/src-tauri/src/storage/migration_store.rs index 7c8804d..3cc8699 100644 --- a/app/src-tauri/src/storage/migration_store.rs +++ b/app/src-tauri/src/storage/migration_store.rs @@ -82,22 +82,41 @@ pub fn load(project_id: &str) -> Result, String> { match serde_json::from_str::(&data) { Ok(state) => Ok(Some(state)), Err(e) => { + // Three outcomes, and they must not be conflated: a copy was made, + // a copy was deliberately not made, or a copy failed. The previous + // version folded "already kept enough" into `Ok(())` and then told + // the user "a copy was kept at " — naming a file that was + // never created. A message that invents a backup is worse than no + // message, because it is what someone reads before going to look + // for their data. let backup = corrupt_backup_path(&path, &chrono::Utc::now()); - let copied = if backup.exists() || corrupt_backups_full(&path) { - // Already kept a copy of this exact corruption this second, or - // kept as many as are worth keeping. Either way nothing to add. - Ok(()) + let kept = if backup.exists() { + Kept::AlreadyThere + } else if corrupt_backups_full(&path) { + Kept::EnoughAlready(MAX_CORRUPT_BACKUPS) } else { - fs::copy(&path, &backup).map(|_| ()) + match fs::copy(&path, &backup) { + Ok(_) => Kept::Copied, + Err(e) => Kept::Failed(e.to_string()), + } }; log::error!( "Failed to parse migration state for project {}: {} — treating as absent, but \ the record is left in place so `has_record` still protects its rollback pin{}", project_id, e, - match copied { - Ok(()) => format!(" (a copy was kept at {})", backup.display()), - Err(ref e) => format!(" (could not keep a copy: {})", e), + match kept { + Kept::Copied | Kept::AlreadyThere => + format!(" (a copy is at {})", backup.display()), + // The earliest copies are the ones worth having, so the cap + // keeps those and drops this one. Say so, rather than + // implying a file exists. + Kept::EnoughAlready(n) => format!( + " (no copy kept — {} earlier copies of this record are already saved \ + alongside it)", + n + ), + Kept::Failed(ref e) => format!(" (could not keep a copy: {})", e), } ); Ok(None) @@ -105,6 +124,17 @@ pub fn load(project_id: &str) -> Result, String> { } } +/// What [`load`] did about a copy of an unparseable record, so the log line can +/// tell the truth about whether a file exists. +enum Kept { + Copied, + /// This exact second's copy was already on disk. + AlreadyThere, + /// The cap is reached; the earlier copies are kept and this one is not. + EnoughAlready(usize), + Failed(String), +} + /// Where a copy of an unparseable record is kept. /// /// Timestamped rather than a fixed `.bak`: a second corruption used to @@ -440,4 +470,43 @@ mod tests { "ab62cd24-51aa-4645-8f5c-17a124062050" ); } + + /// The log line must not name a backup that was never written. + /// + /// `load` runs on every reconcile, every survey and every reaper pass, so a + /// persistently corrupt record hits the `MAX_CORRUPT_BACKUPS` cap within + /// seconds. The previous code folded "already kept enough" into `Ok(())` + /// and then reported " (a copy was kept at )" — pointing at a file + /// that does not exist. That is the message someone reads immediately + /// before going to look for their data. + #[test] + fn the_corrupt_record_message_only_claims_a_copy_that_exists() { + let dir = std::env::temp_dir().join(format!( + "tc-mstore-{}", + uuid::Uuid::new_v4().simple() + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("p.json"); + std::fs::write(&path, b"{ not json").unwrap(); + + // Fill the cap with copies that really are on disk. + for i in 0..MAX_CORRUPT_BACKUPS { + let b = path.with_extension(format!("json.corrupt-2026010{}-000000.bak", i)); + std::fs::write(&b, b"{ not json").unwrap(); + } + assert!(corrupt_backups_full(&path), "precondition: the cap is reached"); + + // With the cap reached, no new copy may be created — and that is the + // state in which the old message lied. + let before: Vec<_> = std::fs::read_dir(&dir).unwrap().flatten().collect(); + let fresh = corrupt_backup_path(&path, &chrono::Utc::now()); + assert!( + !fresh.exists(), + "the cap is reached, so this timestamped copy must not be written" + ); + let after: Vec<_> = std::fs::read_dir(&dir).unwrap().flatten().collect(); + assert_eq!(before.len(), after.len(), "nothing new appeared on disk"); + + std::fs::remove_dir_all(&dir).ok(); + } }