fix(viewer): real errors, no leftover temp files, refuse read-only saves

Fix round 1 for Task 3, addressing task-3-review.md's I1-I3 (reproduced
under dash) plus M3 and M10 from the same review.

- I1: WRITE_SCRIPT read the target's hash through `sha256sum | cut … ||
  exit 1`. POSIX sh has no pipefail, so that `|| exit 1` tested only
  cut's exit status — an unreadable target (EACCES, EIO) left $actual
  empty, which never equals $expect, so the script silently reported
  exit 3 (conflict) instead of a real error. The user got a misleading
  "changed on disk" banner whose "Overwrite on save" could never
  succeed, since the next poll hit the same read error. Fixed by
  reading the hash from a plain command substitution
  (`actual=$(sha256sum -- "$target") || exit 1`) and splitting out the
  hash field in shell instead of piping into `cut`.

- I2 (+ M3): a failed `cp` into the staged file (ENOSPC, quota, EFBIG,
  EIO) left a partial `.<name>.triple-c-<pid>` behind in the user's
  own directory — the EXIT trap only ever removed $tmp. Fixed by
  creating the staged file with `mktemp` (M3: exclusive, unpredictable
  name, so it can't be planted or follow an existing symlink) and
  adding it to the trap as soon as it's assigned
  (`trap 'rm -f -- "$tmp" ${staged:+"$staged"}' EXIT`), so any later
  failure — cp, chmod, mv, or a signal — cleans it up too.

- I3 (controller ruling): the script only ever checked `[ -w "$dir" ]`,
  so a 0444 file (or one owned by another uid) was silently replaced
  via rename, defeating the file's own write protection even though
  spec §5 step 3 reads that way literally. Added `[ -w "$target" ]`
  before the branch; a non-writable target is refused with "The file
  is read-only for the container user." on stderr and a distinct exit
  code (5, `EXIT_READ_ONLY`) that `classify_write` maps to that same
  message rather than falling into the generic clipped-stderr arm.

- M10: added six `#[cfg(unix)]` tests that run WRITE_SCRIPT for real
  via `sh -c` against a temp directory on the host (not just needle
  matches against the script text) — clean save, stale-base conflict,
  gone target, unreadable target (I1), read-only target (I3), and a
  failed stage leaving no partial file behind (I2). The unreadable/
  read-only tests self-skip with a message if permission bits turn out
  not to block root, rather than false-failing under a root test
  runner.

Verified: `cargo test --offline file_viewer` — 19/19 passing, pristine
(up from 12; 6 new host-execution tests plus 1 for the new exit-5
classify_write arm). `cargo clippy --offline` (and `--tests`) — no
warnings in file_viewer::write; the 28 warnings clippy reports are
all pre-existing, in unrelated files.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

app/src-tauri/src/file_viewer/write.rs

@@ -25,29 +25,56 @@ pub fn is_sha256_hex(s: &str) -> bool {
 }
 
 /// `$1` target, `$2` staged payload in /tmp, `$3` the hash the editor loaded from.
-/// Exit 3 = changed on disk, 4 = gone; stdout on success is `sha256sum` of the target.
+/// Exit 1 = a step failed (unreadable target, a failed stage/replace, …), 3 = changed
+/// on disk, 4 = gone, 5 = the target is not writable by the container user; stdout on
+/// success is `sha256sum` of the target.
 ///
 /// P15: `sha256sum -- "$target"` prefixes its whole line with `\` when the path
 /// contains a backslash or a newline, so `$actual` has that prefix stripped before
 /// it is compared with `$expect` (which never carries one) — otherwise such a path
 /// would conflict forever.
+///
+/// I1: `$actual` is read from a plain `sha256sum` command substitution, not a
+/// pipeline into `cut` — POSIX sh has no `pipefail`, so `cmd | cut … || exit 1` tests
+/// only `cut`'s exit status and an unreadable file (EACCES, EIO) fell through as a
+/// false "changed on disk" conflict (empty `$actual` never equals `$expect`) instead
+/// of a real error, hiding the actual failure from the user and from `classify_write`.
+///
+/// I2/M3: `$staged` is created by `mktemp` (exclusive — never follows a planted
+/// symlink or stale leftover at that name) and is part of the `EXIT` trap from the
+/// moment it is assigned, so a failure at any later step (`cp`, `chmod`, `mv`) cannot
+/// leave a partial `.<name>.triple-c-<suffix>` behind in the user's own directory —
+/// including on a signal, for the steps after the trap covers it.
 pub const WRITE_SCRIPT: &str = r#"target=$1; tmp=$2; expect=$3
-trap 'rm -f -- "$tmp"' EXIT
+staged=
+trap 'rm -f -- "$tmp" ${staged:+"$staged"}' EXIT
 test -f "$target" || exit 4
-actual=$(sha256sum -- "$target" | cut -d' ' -f1) || exit 1
+actual=$(sha256sum -- "$target") || exit 1
-actual=${actual#\\}
+actual=${actual%% *}; actual=${actual#\\}
 [ "$actual" = "$expect" ] || exit 3
+# I3: the file's own mode is a boundary the user set from outside the container (0444,
+# a different owning uid, a read-only bind mount, …). Replacing it via rename or
+# truncating it in place would silently cross that boundary even though `claude` is
+# allowed to — an editor such as vim, or a plain `echo > file` in the user's own shell,
+# would refuse. This is stricter than spec §5 step 3's literal "if the directory is
+# writable" branch, which never looks at the file's own permissions; the branch below
+# only ever chooses *how* to write, never *whether*.
+[ -w "$target" ] || { echo "The file is read-only for the container user." >&2; exit 5; }
 dir=$(dirname -- "$target"); name=$(basename -- "$target")
 if [ -w "$dir" ]; then
-  staged="$dir/.$name.triple-c-$$"
+  staged=$(mktemp -- "$dir/.$name.triple-c-XXXXXX") || exit 1
   cp -- "$tmp" "$staged" || exit 1
   chmod --reference="$target" "$staged" 2>/dev/null
-  mv -f -- "$staged" "$target" || { rm -f -- "$staged"; exit 1; }
+  mv -f -- "$staged" "$target" || exit 1
 else
   cat -- "$tmp" > "$target" || exit 1
 fi
 sha256sum -- "$target""#;
 
+/// I3: distinct from the generic failure code so the caller can hand back a specific,
+/// readable message instead of whatever the script's own diagnostic text says.
+const EXIT_READ_ONLY: i64 = 5;
+
 pub enum WriteOutcome {
     Saved(String),
     Conflict,
@@ -59,6 +86,9 @@ pub fn classify_write(code: i64, stdout: &str, stderr: &str) -> WriteOutcome {
     match code {
         3 => WriteOutcome::Conflict,
         4 => WriteOutcome::Gone,
+        EXIT_READ_ONLY => {
+            WriteOutcome::Failed("The file is read-only for the container user.".into())
+        }
         0 => match stdout
             .split_whitespace()
             .next()
@@ -152,6 +182,17 @@ mod tests {
         assert!(matches!(classify_write(0, "junk", ""), WriteOutcome::Failed(_)));
     }
 
+    /// I3: exit 5 is the script's read-only refusal, and it must not be swallowed by
+    /// the generic `_ => Failed(stderr)` arm — the caller gets a fixed, readable
+    /// message regardless of exactly what the script printed.
+    #[test]
+    fn exit_five_is_a_distinct_read_only_refusal() {
+        assert!(matches!(
+            classify_write(5, "", "The file is read-only for the container user."),
+            WriteOutcome::Failed(m) if m.contains("read-only")
+        ));
+    }
+
     /// P15: a target path with a backslash makes `sha256sum` prefix the line;
     /// the parsed hash must still be recognised as the saved hash.
     #[test]
@@ -171,10 +212,18 @@ mod tests {
             "chmod --reference=\"$target\"",
             "mv -f --",
             "cat -- \"$tmp\" > \"$target\"",
-            "trap 'rm -f -- \"$tmp\"' EXIT",
+            // I2/M3: the trap covers the staged file too, and it comes from `mktemp`.
+            "trap 'rm -f -- \"$tmp\" ${staged:+\"$staged\"}' EXIT",
+            "mktemp -- \"$dir/.$name.triple-c-XXXXXX\"",
+            // I1: a plain command substitution, not a pipeline `cut` could mask.
+            "actual=$(sha256sum -- \"$target\") || exit 1",
+            // I3: a read-only target is refused before any write is attempted.
+            "[ -w \"$target\" ] || { echo \"The file is read-only for the container user.\" >&2; exit 5; }",
         ] {
             assert!(WRITE_SCRIPT.contains(needle), "missing: {}", needle);
         }
+        // The old pipeline form must be gone, not merely superseded.
+        assert!(!WRITE_SCRIPT.contains("cut -d' ' -f1"));
     }
 
     /// P8: the write script's test list is binding, and the argument order is
@@ -205,4 +254,203 @@ mod tests {
         assert!(check_write_input(MAX_WRITE_BYTES + 1, h).is_err());
         assert!(check_write_input(0, "not-a-hash").is_err());
     }
+
+    // ── M10: WRITE_SCRIPT run for real, against a temp dir on the host ──────────
+    //
+    // The needle test above only proves the script *contains* certain substrings; it
+    // cannot catch the pipefail-shaped bug I1 was (the needle text was correct, the
+    // shell semantics were not). These run the exact `sh -c SCRIPT save target tmp
+    // hash` invocation `write_command` builds, so they pin the exit codes and cleanup
+    // behaviour that `write_file`/`classify_write` actually depend on. `sh` and the
+    // coreutils used here (`sha256sum`, `mktemp`, `dirname`, `basename`) are present
+    // on dev machines and CI alike.
+
+    #[cfg(unix)]
+    fn run_write_script(
+        target: &std::path::Path,
+        tmp: &std::path::Path,
+        base_hash: &str,
+    ) -> (i32, String, String) {
+        let out = std::process::Command::new("sh")
+            .arg("-c")
+            .arg(WRITE_SCRIPT)
+            .arg("save")
+            .arg(target)
+            .arg(tmp)
+            .arg(base_hash)
+            .output()
+            .expect("sh must be on PATH to run this test");
+        (
+            out.status.code().unwrap_or(-1),
+            String::from_utf8_lossy(&out.stdout).into_owned(),
+            String::from_utf8_lossy(&out.stderr).into_owned(),
+        )
+    }
+
+    #[cfg(unix)]
+    fn unique_test_dir(name: &str) -> std::path::PathBuf {
+        let dir = std::env::temp_dir().join(format!("tc-write-{}-{}", name, uuid::Uuid::new_v4()));
+        std::fs::create_dir_all(&dir).unwrap();
+        dir
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_a_clean_save_replaces_the_file_and_cleans_up() {
+        let dir = unique_test_dir("clean");
+        let target = dir.join("t.txt");
+        let tmp = dir.join("payload");
+        std::fs::write(&target, b"old\n").unwrap();
+        std::fs::write(&tmp, b"new\n").unwrap();
+        let base = sha256_hex(b"old\n");
+
+        let (code, stdout, stderr) = run_write_script(&target, &tmp, &base);
+
+        assert_eq!(code, 0, "stdout={stdout} stderr={stderr}");
+        let new_hash = sha256_hex(b"new\n");
+        assert!(stdout.contains(&new_hash), "stdout={stdout}");
+        assert_eq!(std::fs::read(&target).unwrap(), b"new\n");
+        assert!(!tmp.exists(), "the staged /tmp payload must be cleaned up");
+
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_a_stale_base_hash_conflicts_and_leaves_everything_untouched() {
+        let dir = unique_test_dir("stale");
+        let target = dir.join("t.txt");
+        let tmp = dir.join("payload");
+        std::fs::write(&target, b"old\n").unwrap();
+        std::fs::write(&tmp, b"new\n").unwrap();
+        let wrong_base = sha256_hex(b"not what is on disk\n");
+
+        let (code, _stdout, stderr) = run_write_script(&target, &tmp, &wrong_base);
+
+        assert_eq!(code, 3, "stderr={stderr}");
+        assert_eq!(std::fs::read(&target).unwrap(), b"old\n", "must be untouched");
+        assert!(!tmp.exists(), "the staged /tmp payload must still be cleaned up");
+
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_a_missing_target_reports_gone() {
+        let dir = unique_test_dir("gone");
+        let target = dir.join("does-not-exist");
+        let tmp = dir.join("payload");
+        std::fs::write(&tmp, b"new\n").unwrap();
+
+        let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"whatever"));
+
+        assert_eq!(code, 4, "stderr={stderr}");
+
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    /// I1: a real read failure must be a real error (exit 1), never the exit-3
+    /// conflict a bare `sha256sum | cut` pipeline (no `pipefail` in POSIX sh) would
+    /// silently produce.
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_an_unreadable_target_is_an_error_not_a_conflict() {
+        use std::os::unix::fs::PermissionsExt;
+        let dir = unique_test_dir("unreadable");
+        let target = dir.join("t.txt");
+        let tmp = dir.join("payload");
+        std::fs::write(&target, b"old\n").unwrap();
+        std::fs::write(&tmp, b"new\n").unwrap();
+        std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o000)).unwrap();
+
+        if std::fs::read(&target).is_ok() {
+            // Running as root (or some other bypass): 0o000 does not block reads,
+            // so this scenario cannot be reproduced here.
+            eprintln!("skipping: still able to read a 0o000 file (root?)");
+            let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644));
+            let _ = std::fs::remove_dir_all(&dir);
+            return;
+        }
+
+        let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"old\n"));
+
+        assert_eq!(
+            code, 1,
+            "an unreadable target must be a real error, not exit 3; stderr={stderr}"
+        );
+        assert!(!tmp.exists(), "the staged /tmp payload must still be cleaned up");
+
+        let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644));
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    /// I3: a target the container user cannot write is refused outright, never
+    /// replaced via rename.
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_a_read_only_target_is_refused_not_replaced() {
+        use std::os::unix::fs::PermissionsExt;
+        let dir = unique_test_dir("readonly");
+        let target = dir.join("t.txt");
+        let tmp = dir.join("payload");
+        std::fs::write(&target, b"old\n").unwrap();
+        std::fs::write(&tmp, b"new\n").unwrap();
+        std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o444)).unwrap();
+
+        if std::fs::OpenOptions::new().write(true).open(&target).is_ok() {
+            eprintln!("skipping: still able to write a 0o444 file (root?)");
+            let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644));
+            let _ = std::fs::remove_dir_all(&dir);
+            return;
+        }
+
+        let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"old\n"));
+
+        assert_eq!(code as i64, EXIT_READ_ONLY, "stderr={stderr}");
+        assert!(stderr.contains("read-only"), "stderr={stderr}");
+        assert_eq!(
+            std::fs::read(&target).unwrap(),
+            b"old\n",
+            "a read-only file must not be replaced"
+        );
+        assert!(!tmp.exists(), "the staged /tmp payload must still be cleaned up");
+
+        let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644));
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    /// I2: a failed stage (here: an unreadable source payload, so `cp` fails after
+    /// `mktemp` has already created the destination) must not leave a partial
+    /// `.<name>.triple-c-<suffix>` behind in the user's own directory.
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_a_failed_stage_leaves_no_partial_file_behind() {
+        use std::os::unix::fs::PermissionsExt;
+        let dir = unique_test_dir("cpfail");
+        let target = dir.join("t.txt");
+        let tmp = dir.join("payload");
+        std::fs::write(&target, b"old\n").unwrap();
+        std::fs::write(&tmp, b"new\n").unwrap();
+        std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o000)).unwrap();
+
+        if std::fs::read(&tmp).is_ok() {
+            eprintln!("skipping: still able to read a 0o000 file (root?)");
+            let _ = std::fs::remove_dir_all(&dir);
+            return;
+        }
+
+        let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"old\n"));
+
+        assert_eq!(code, 1, "stderr={stderr}");
+        assert_eq!(std::fs::read(&target).unwrap(), b"old\n", "must be untouched");
+        let leftovers: Vec<_> = std::fs::read_dir(&dir)
+            .unwrap()
+            .filter_map(|e| e.ok())
+            .map(|e| e.file_name().to_string_lossy().into_owned())
+            .filter(|n| n.starts_with(".t.txt.triple-c-"))
+            .collect();
+        assert!(leftovers.is_empty(), "staged file(s) left behind: {leftovers:?}");
+
+        let _ = std::fs::remove_dir_all(&dir);
+    }
 }