From 168b61d6328c34fc40b296bd9d361883702825d1 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 15:52:49 -0700 Subject: [PATCH] Close two cross-file handoffs from the round-3 fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The terminal's drop target derived the tar entry name from the host path *after* symlink resolution, so dropping ~/Downloads/latest.log — where latest.log is a symlink — landed the file in the container under the target's name. Nothing errored; the user got a name they never typed. The Files pane had the identical bug and was fixed with `host_upload_name`; the terminal now calls the same helper, so the two drop targets cannot drift. It also drops the "dropped-file" fallback, which silently renamed anything the old `file_name()` could not parse. `migration_store::load` told the user a backup existed when it had deliberately not written one. `load` runs on every reconcile, survey and reaper pass, so a persistently corrupt record reaches MAX_CORRUPT_BACKUPS within seconds; from then on the copy was skipped while the log still read "(a copy was kept at )". Three outcomes are now distinct, and the "enough already" case says so rather than naming a file that is not there — that being the message someone reads immediately before going to look for their data. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- app/src-tauri/src/commands/file_commands.rs | 2 +- .../src/commands/terminal_commands.rs | 48 +++++++++-- app/src-tauri/src/storage/migration_store.rs | 85 +++++++++++++++++-- 3 files changed, 121 insertions(+), 14 deletions(-) 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(); + } }