fix: address codex-review P2 findings (upload race, compare dupes, annotated clip)

CHANGELOG.md

@@ -91,6 +91,21 @@ All notable changes to this project are documented here.
 
 ### Fixed
 
+- **Upload never clobbers a concurrently-created file (codex review P2).**
+  The `upload` write replaced a check-then-write (`exists()` then
+  `os.replace`/`rename` — which could overwrite a file created in the race
+  window) with an atomic hard-link into place that fails with an "already
+  exists" error instead. Fixed in both the Rust companion
+  (`fsutil::write_new`) and the Python bridge (`os.link`).
+- **`compare` rejects duplicate variant `value`s (codex review P2).** Two
+  variants sharing a `value` collided as the keyed-`{#each}` key and as the
+  returned `selected`; `validate_spec` now rejects duplicates before render,
+  covering both the native and remote (Python) paths.
+- **`annotated_image` scales the image into `max_height` instead of clipping
+  it (codex review P2).** The height cap now applies to the image itself, not
+  just the overflow-hidden stage, so the whole image stays visible and click
+  coordinates normalize correctly (a click near the visible bottom previously
+  mapped to the wrong `y`).
 - **Reconciled skill.md drift between the two shipped copies + added a CI
   drift guard.** The canonical agent skill (`docs/skill.md`, embedded in
   the native Rust MCP server) and the copy shipped with the Python bridge

companion/src-tauri/src/fsutil.rs

@@ -42,6 +42,38 @@ pub fn atomic_write(path: &Path, content: &[u8]) -> std::io::Result<()> {
     Ok(())
 }
 
+/// Like [`atomic_write`], but never overwrites an existing file. Writes
+/// `content` to a sibling temp, fsyncs it, then hard-links it into place:
+/// the link is atomic and fails with [`std::io::ErrorKind::AlreadyExists`]
+/// if the destination already exists, closing the check-then-write race a
+/// plain `exists()` guard leaves open. The temp is always cleaned up. Used
+/// by the upload tool, whose contract is a deterministic destination that
+/// must not clobber the user's data even under a concurrent create (codex
+/// review P2).
+pub fn write_new(path: &Path, content: &[u8]) -> std::io::Result<()> {
+    let parent = path.parent().unwrap_or_else(|| Path::new("."));
+    fs::create_dir_all(parent)?;
+    let tmp = path.with_extension(format!(
+        "tmp.{}.{}",
+        std::process::id(),
+        std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .map(|d| d.as_nanos())
+            .unwrap_or(0)
+    ));
+    {
+        let mut f = fs::OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(&tmp)?;
+        f.write_all(content)?;
+        f.sync_all()?;
+    }
+    let res = fs::hard_link(&tmp, path);
+    let _ = fs::remove_file(&tmp);
+    res
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -58,6 +90,20 @@ mod tests {
         let _ = fs::remove_dir_all(&dir);
     }
 
+    #[test]
+    fn write_new_refuses_to_clobber() {
+        let dir = std::env::temp_dir().join(format!("aiui-fsutil-wn-{}", std::process::id()));
+        let _ = fs::create_dir_all(&dir);
+        let target = dir.join("b.txt");
+        write_new(&target, b"first").unwrap();
+        assert_eq!(fs::read_to_string(&target).unwrap(), "first");
+        // Second write to an existing path must fail atomically, not clobber.
+        let err = write_new(&target, b"second").unwrap_err();
+        assert_eq!(err.kind(), std::io::ErrorKind::AlreadyExists);
+        assert_eq!(fs::read_to_string(&target).unwrap(), "first");
+        let _ = fs::remove_dir_all(&dir);
+    }
+
     #[test]
     fn no_temp_files_left_behind_on_success() {
         let dir = std::env::temp_dir().join(format!(

companion/src-tauri/src/http.rs

@@ -836,18 +836,32 @@ fn validate_spec(spec: &serde_json::Value) -> Result<(), (String, String)> {
                 ));
             }
             Some(arr) => {
+                let mut seen = std::collections::HashSet::new();
                 for (i, it) in arr.iter().enumerate() {
-                    let has_value = it
+                    let value = it
                         .get("value")
                         .and_then(|v| v.as_str())
-                        .map(|s| !s.is_empty())
-                        .unwrap_or(false);
-                    if !has_value {
-                        return Err((
-                            format!("compare variant #{i} is missing a non-empty 'value'"),
-                            "Each variant needs a stable 'value' string — it's returned as 'selected' when picked."
-                                .into(),
-                        ));
+                        .filter(|s| !s.is_empty());
+                    match value {
+                        None => {
+                            return Err((
+                                format!("compare variant #{i} is missing a non-empty 'value'"),
+                                "Each variant needs a stable 'value' string — it's returned as 'selected' when picked."
+                                    .into(),
+                            ));
+                        }
+                        // Duplicate values collide as the keyed-`{#each}` key and
+                        // as the returned `selected`, making two options
+                        // indistinguishable — reject before render (codex review P2).
+                        Some(v) => {
+                            if !seen.insert(v) {
+                                return Err((
+                                    format!("compare has a duplicate variant 'value': {v:?}"),
+                                    "Each variant's 'value' must be unique — it keys the rendered list and the returned selection."
+                                        .into(),
+                                ));
+                            }
+                        }
                     }
                 }
             }
@@ -1403,6 +1417,27 @@ mod validate_tests {
         assert!(validate_spec(&spec).is_ok());
     }
 
+    #[test]
+    fn accepts_compare_with_unique_values() {
+        let spec = json!({"kind":"compare","variants":[
+            {"value":"a","content":"Draft A"},
+            {"value":"b","content":"Draft B"}
+        ]});
+        assert!(validate_spec(&spec).is_ok());
+    }
+
+    #[test]
+    fn rejects_compare_with_duplicate_values() {
+        // Duplicate variant values collide as the keyed-`{#each}` key and the
+        // returned `selected` — must be rejected before render (codex review P2).
+        let spec = json!({"kind":"compare","variants":[
+            {"value":"a","content":"Draft A"},
+            {"value":"a","content":"Draft A prime"}
+        ]});
+        let err = validate_spec(&spec).unwrap_err();
+        assert!(err.0.contains("duplicate"));
+    }
+
     #[test]
     fn accepts_form_with_tabs() {
         let spec = json!({"kind":"form","tabs":[

companion/src-tauri/src/mcp.rs

@@ -1029,15 +1029,18 @@ async fn do_upload(args: &Value, cfg: &AppConfig, http: &reqwest::Client) -> Val
 
     let dest = target_dir.join(&filename);
     // Never clobber: a deterministic path is the point, but silently
-    // overwriting the user's existing file is not. Fail loudly instead.
-    if dest.exists() {
-        return upload_error(format!(
-            "target already exists, not overwriting: {}",
-            dest.display()
-        ));
-    }
-    if let Err(e) = crate::fsutil::atomic_write(&dest, &bytes) {
-        return upload_error(format!("writing {}: {e}", dest.display()));
+    // overwriting the user's existing file is not. `write_new` links the
+    // file into place atomically, so a file created between here and the
+    // write cannot be lost — no check-then-write race (codex review P2).
+    match crate::fsutil::write_new(&dest, &bytes) {
+        Ok(()) => {}
+        Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
+            return upload_error(format!(
+                "target already exists, not overwriting: {}",
+                dest.display()
+            ));
+        }
+        Err(e) => return upload_error(format!("writing {}: {e}", dest.display())),
     }
 
     value_to_tool_text(json!({

companion/src/lib/widgets/Form.svelte

@@ -650,7 +650,6 @@
           <div
             class="annimg-stage"
             class:region-tool={annActiveTool(f) === "region"}
-            style={f.max_height ? `max-height: ${f.max_height}px` : ""}
             onpointerdown={(e) => annPointerDown(f, e, e.currentTarget as HTMLElement)}
             onpointermove={(e) => annPointerMove(f, e, e.currentTarget as HTMLElement)}
             onpointerup={(e) => annPointerUp(f, e, e.currentTarget as HTMLElement)}
@@ -660,6 +659,7 @@
               src={f.src}
               alt={f.alt ?? f.label ?? ""}
               draggable="false"
+              style={f.max_height ? `max-height: ${f.max_height}px` : ""}
               onload={(e) => annOnImageLoad(f.name, e.currentTarget as HTMLImageElement)}
             />
             <svg

python/src/aiui_mcp/server.py

@@ -665,20 +665,25 @@ def _upload_write(dest_dir: Path, filename: str, data: bytes) -> dict[str, Any]:
     Rust bridge. Returns the `{status, …}` payload.
     """
     dest = dest_dir / filename
-    if dest.exists():
-        return {"status": "error", "error": f"target already exists, not overwriting: {dest}"}
     try:
         fd, tmp = tempfile.mkstemp(prefix=".aiui-upload-", dir=str(dest_dir))
         try:
             with os.fdopen(fd, "wb") as f:
                 f.write(data)
-            os.replace(tmp, dest)
-        except BaseException:
+                f.flush()
+                os.fsync(f.fileno())
+            # Hard-link into place: atomic, and raises FileExistsError if the
+            # destination already exists — no check-then-write race window a
+            # prior `exists()` guard + `os.replace` left open (codex review P2).
+            try:
+                os.link(tmp, dest)
+            except FileExistsError:
+                return {"status": "error", "error": f"target already exists, not overwriting: {dest}"}
+        finally:
             try:
                 os.unlink(tmp)
             except OSError:
                 pass
-            raise
     except OSError as e:
         return {"status": "error", "error": f"writing {dest}: {e}"}
     return {"status": "ok", "path": str(dest), "filename": filename, "bytes": len(data)}