fix(viewer): final-review fixes — save base from written bytes, honest poll errors, retryable first read

- write.rs: a save's new base is sha256 of the bytes written; the script's post-mv hash comes back as disk_hash, and a mismatch (another writer landed after us) shows "Changed on disk" instead of being adopted (ledger M2). - write.rs: conflict:/gone:/read-only strings are constants with a pure saved_file() mapping and tests; app/src/viewer/ipcMessages.ts is the one TS copy and a cargo test checks it against the Rust originals. - write.rs: the comment now says the in-place `cat >` fallback follows a planted symlink, and why that is accepted (runs as claude). - poll.rs: a file deleted between `test -f` and `sha256sum` reads as gone. - viewerState/EditorPane: poll_failed carries its message; only the "Start the project before" refusal reads as Container not running, anything else gets its own banner and leaves Save enabled. - EditorPane: a failed first read shows Retry and is retried by the poll. - spec §1: refused OSC 8 targets keep the refusal card (Task 9 ruling). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2026-09-22 21:52:43 -07:00
parent 398281c8b5
commit bf2291089a
12 changed files with 535 additions and 73 deletions
@@ -7,6 +7,7 @@
 //! A non-root process cannot `chown`, so the saved file is owned by the container user,
 //! as it would be after Claude Code edited it; mode is kept with `chmod --reference`.

+use serde::Serialize;
 use sha2::{Digest, Sha256};

 use crate::commands::file_commands::clip_container_text;
@@ -27,7 +28,10 @@ pub fn is_sha256_hex(s: &str) -> bool {
 /// `$1` target, `$2` staged payload in /tmp, `$3` the hash the editor loaded from.
 /// 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.
+/// success is `sha256sum` of the target *after* the write. That is not necessarily the
+/// hash of what we wrote: another writer (Claude Code, on the same file) can land
+/// between `mv` and `sha256sum`. `saved_file` therefore takes the save's base from the
+/// bytes and only reports this one as what the disk held afterwards (M2).
 ///
 /// P15: `sha256sum -- "$target"` prefixes its whole line with `\` when the path
 /// contains a backslash or a newline, so `$actual` has that prefix stripped before
@@ -59,6 +63,12 @@ actual=${actual%% *}; actual=${actual#\\}
 # 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*.
+#
+# The rename branch replaces whatever is at "$target" (a symlink planted there after
+# the window opened is replaced, not followed). The in-place `cat >` fallback, taken
+# only for a writable file in a read-only directory, DOES follow such a symlink and
+# writes through it. That is accepted: the write runs as `claude`, so it can reach
+# nothing Claude Code in the same container cannot already write.
 [ -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
@@ -71,6 +81,16 @@ else
 fi
 sha256sum -- "$target""#;

+/// A save refused because the file changed since its base hash. The frontend matches
+/// this prefix; its copy lives in `app/src/viewer/ipcMessages.ts` (pinned by a test).
+pub const CONFLICT_PREFIX: &str = "conflict:";
+/// A save refused because the file no longer exists; mirrored in `ipcMessages.ts`.
+pub const GONE_PREFIX: &str = "gone:";
+/// The read-only refusal. The script echoes the same sentence (pinned by a test), but
+/// the caller always gets this constant, whatever the script printed; mirrored in
+/// `ipcMessages.ts`.
+pub const READ_ONLY_MESSAGE: &str = "The file is read-only for the container user.";
+
 /// 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;
@@ -86,9 +106,7 @@ 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())
-        }
+        EXIT_READ_ONLY => WriteOutcome::Failed(READ_ONLY_MESSAGE.into()),
        0 => match stdout
            .split_whitespace()
            .next()
@@ -130,13 +148,36 @@ fn check_write_input(len: usize, base_hash: &str) -> Result<(), String> {
    Ok(())
 }

+/// What a successful save reports: `hash` is the new base, `sha256_hex` of the bytes
+/// we wrote; `disk_hash` is what the container hashed right after the swap. They differ
+/// only when another writer landed in between, and then the editor must show "Changed
+/// on disk" rather than adopt the other writer's hash as its base (M2).
+#[derive(Clone, Debug, Serialize, PartialEq, Eq)]
+pub struct SavedFile {
+    pub hash: String,
+    pub disk_hash: String,
+}
+
+/// `viewer_write_file`'s result, pure so the error-prefix contract has a unit test.
+fn saved_file(outcome: WriteOutcome, bytes: &[u8]) -> Result<SavedFile, String> {
+    match outcome {
+        WriteOutcome::Saved(disk_hash) => Ok(SavedFile { hash: sha256_hex(bytes), disk_hash }),
+        WriteOutcome::Conflict => Err(format!(
+            "{} the file changed on disk since it was loaded.",
+            CONFLICT_PREFIX
+        )),
+        WriteOutcome::Gone => Err(format!("{} the file no longer exists.", GONE_PREFIX)),
+        WriteOutcome::Failed(msg) => Err(format!("Could not save the file: {}", msg)),
+    }
+}
+
 pub async fn write_file(
    container_id: &str,
    exec_manager: &ExecSessionManager,
    target: &str,
    bytes: &[u8],
    base_hash: &str,
-) -> Result<String, String> {
+) -> Result<SavedFile, String> {
    check_write_input(bytes.len(), base_hash)?;
    let tmp_name = format!("triple-c-viewer-{}", uuid::Uuid::new_v4().simple());
    let tmp_path = exec_manager
@@ -145,14 +186,7 @@ pub async fn write_file(
    let cmd = write_command(target, &tmp_path, base_hash);
    let (stdout, stderr, code) =
        exec_oneshot_streams_as(container_id, "claude", cmd, Vec::new()).await?;
-    match classify_write(code, &stdout, &stderr) {
-        WriteOutcome::Saved(hash) => Ok(hash),
-        WriteOutcome::Conflict => {
-            Err("conflict: the file changed on disk since it was loaded.".into())
-        }
-        WriteOutcome::Gone => Err("gone: the file no longer exists.".into()),
-        WriteOutcome::Failed(msg) => Err(format!("Could not save the file: {}", msg)),
-    }
+    saved_file(classify_write(code, &stdout, &stderr), bytes)
 }

 #[cfg(test)]
@@ -193,6 +227,60 @@ mod tests {
        ));
    }

+    /// M2: the new base is the hash of the bytes we wrote, never the script's
+    /// post-`mv` hash, which may belong to a writer that landed after us.
+    #[test]
+    fn a_save_takes_its_base_from_the_written_bytes() {
+        let ours = sha256_hex(b"new\n");
+        let same = saved_file(WriteOutcome::Saved(ours.clone()), b"new\n").unwrap();
+        assert_eq!(same, SavedFile { hash: ours.clone(), disk_hash: ours.clone() });
+
+        let foreign = sha256_hex(b"someone else's\n");
+        let raced = saved_file(WriteOutcome::Saved(foreign.clone()), b"new\n").unwrap();
+        assert_eq!(raced.hash, ours, "the base must be what we wrote");
+        assert_eq!(raced.disk_hash, foreign, "the foreign hash is reported, not adopted");
+    }
+
+    /// Important #4: the frontend matches these exact strings
+    /// (`app/src/viewer/ipcMessages.ts`), so pin them here too.
+    #[test]
+    fn save_errors_keep_the_prefix_contract() {
+        let conflict = saved_file(WriteOutcome::Conflict, b"").unwrap_err();
+        assert!(conflict.starts_with("conflict:"), "{conflict}");
+        assert_eq!(conflict, "conflict: the file changed on disk since it was loaded.");
+
+        let gone = saved_file(WriteOutcome::Gone, b"").unwrap_err();
+        assert!(gone.starts_with("gone:"), "{gone}");
+        assert_eq!(gone, "gone: the file no longer exists.");
+
+        let read_only = saved_file(classify_write(5, "", "whatever the script said"), b"").unwrap_err();
+        assert_eq!(read_only, "Could not save the file: The file is read-only for the container user.");
+        assert!(!read_only.starts_with(CONFLICT_PREFIX) && !read_only.starts_with(GONE_PREFIX));
+
+        let other = saved_file(classify_write(1, "", "No space left on device"), b"").unwrap_err();
+        assert_eq!(other, "Could not save the file: No space left on device");
+
+        // The script's own refusal text is the same sentence the caller is given.
+        assert!(WRITE_SCRIPT.contains(&format!("echo \"{}\" >&2; exit 5", READ_ONLY_MESSAGE)));
+    }
+
+    /// The TypeScript side keeps one copy of each matched string; a change on either
+    /// side without the other fails here.
+    #[test]
+    fn the_frontend_copies_of_the_ipc_messages_match() {
+        let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../src/viewer/ipcMessages.ts");
+        let ts = std::fs::read_to_string(&path).expect("app/src/viewer/ipcMessages.ts");
+        for (name, value) in [
+            ("CONFLICT_PREFIX", CONFLICT_PREFIX),
+            ("GONE_PREFIX", GONE_PREFIX),
+            ("READ_ONLY_MESSAGE", READ_ONLY_MESSAGE),
+            ("NOT_RUNNING_PREFIX", crate::commands::file_commands::NOT_RUNNING_PREFIX),
+        ] {
+            let line = format!("export const {} = \"{}\";", name, value);
+            assert!(ts.contains(&line), "ipcMessages.ts must contain `{line}`");
+        }
+    }
+
    /// P15: a target path with a backslash makes `sha256sum` prefix the line;
    /// the parsed hash must still be recognised as the saved hash.
    #[test]
@@ -309,12 +397,72 @@ mod tests {
        assert_eq!(code, 0, "stdout={stdout} stderr={stderr}");
        let new_hash = sha256_hex(b"new\n");
        assert!(stdout.contains(&new_hash), "stdout={stdout}");
+        // With no other writer, the reported disk hash is ours, so no conflict is shown.
+        let saved = saved_file(classify_write(code as i64, &stdout, &stderr), b"new\n").unwrap();
+        assert_eq!(saved, SavedFile { hash: new_hash.clone(), disk_hash: new_hash.clone() });
        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);
    }

+    /// M2, for real: another writer lands between the script's `mv` and its final
+    /// `sha256sum` (simulated by a `sha256sum` shim on PATH that rewrites the target on
+    /// its second call). The save's base must still be the hash of our bytes, and the
+    /// foreign hash must come back as `disk_hash`, so the editor shows "Changed on disk".
+    #[cfg(unix)]
+    #[test]
+    fn on_the_host_a_write_that_lands_after_ours_is_reported_not_adopted() {
+        use std::os::unix::fs::PermissionsExt;
+        let real = std::process::Command::new("sh")
+            .args(["-c", "command -v sha256sum"])
+            .output()
+            .expect("sh");
+        let real = String::from_utf8_lossy(&real.stdout).trim().to_string();
+        assert!(!real.is_empty(), "sha256sum must be on PATH");
+
+        let dir = unique_test_dir("race");
+        let bin = dir.join("bin");
+        std::fs::create_dir_all(&bin).unwrap();
+        let mark = dir.join("called-once");
+        let shim = bin.join("sha256sum");
+        std::fs::write(
+            &shim,
+            format!(
+                "#!/bin/sh\nif [ -e '{mark}' ]; then printf 'theirs\\n' > \"$2\"; fi\n: > '{mark}'\nexec '{real}' \"$@\"\n",
+                mark = mark.display(),
+                real = real
+            ),
+        )
+        .unwrap();
+        std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o755)).unwrap();
+
+        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 path = format!("{}:{}", bin.display(), std::env::var("PATH").unwrap_or_default());
+        let out = std::process::Command::new("sh")
+            .env("PATH", path)
+            .arg("-c")
+            .arg(WRITE_SCRIPT)
+            .arg("save")
+            .arg(&target)
+            .arg(&tmp)
+            .arg(sha256_hex(b"old\n"))
+            .output()
+            .unwrap();
+        let (stdout, stderr) = (String::from_utf8_lossy(&out.stdout), String::from_utf8_lossy(&out.stderr));
+        assert_eq!(out.status.code(), Some(0), "stdout={stdout} stderr={stderr}");
+        assert_eq!(std::fs::read(&target).unwrap(), b"theirs\n", "the shim's write landed last");
+
+        let saved = saved_file(classify_write(0, &stdout, &stderr), b"new\n").unwrap();
+        assert_eq!(saved.hash, sha256_hex(b"new\n"));
+        assert_eq!(saved.disk_hash, sha256_hex(b"theirs\n"));
+
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
    #[cfg(unix)]
    #[test]
    fn on_the_host_a_stale_base_hash_conflicts_and_leaves_everything_untouched() {