From bd1c8389297407059521dd450ca34b23eb04efda Mon Sep 17 00:00:00 2001 From: iret77 <63622643+iret77@users.noreply.github.com> Date: Sun, 2 Aug 2026 23:16:24 +0200 Subject: [PATCH] fix: address codex-review P2 findings (upload race, compare dupes, annotated clip) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - upload: atomic no-clobber write (fsutil::write_new / os.link) instead of exists()-then-rename/replace, closing the TOCTOU where a concurrently created file could be overwritten despite the never-overwrite contract. Both the Rust companion and the Python bridge. - compare: validate_spec now rejects duplicate variant 'value's, which otherwise collide as the keyed-{#each} key and the returned 'selected' (covers native + remote via the companion validator). - annotated_image: apply max_height to the so the whole image scales into the stage instead of being clipped by the overflow-hidden stage — clicks near the visible bottom no longer normalize to the wrong y. Adds fsutil write_new no-clobber test + compare dup-value validation tests. --- CHANGELOG.md | 15 ++++++++ companion/src-tauri/src/fsutil.rs | 46 +++++++++++++++++++++++ companion/src-tauri/src/http.rs | 53 ++++++++++++++++++++++----- companion/src-tauri/src/mcp.rs | 21 ++++++----- companion/src/lib/widgets/Form.svelte | 2 +- python/src/aiui_mcp/server.py | 15 +++++--- 6 files changed, 128 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 211cd33..9fd528f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/companion/src-tauri/src/fsutil.rs b/companion/src-tauri/src/fsutil.rs index 1d66dcb..6872c53 100644 --- a/companion/src-tauri/src/fsutil.rs +++ b/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!( diff --git a/companion/src-tauri/src/http.rs b/companion/src-tauri/src/http.rs index cbcc3f8..0c00d9e 100644 --- a/companion/src-tauri/src/http.rs +++ b/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":[ diff --git a/companion/src-tauri/src/mcp.rs b/companion/src-tauri/src/mcp.rs index 3120eb4..e4ea9eb 100644 --- a/companion/src-tauri/src/mcp.rs +++ b/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!({ diff --git a/companion/src/lib/widgets/Form.svelte b/companion/src/lib/widgets/Form.svelte index 4dfe76f..fdc3f5d 100644 --- a/companion/src/lib/widgets/Form.svelte +++ b/companion/src/lib/widgets/Form.svelte @@ -650,7 +650,6 @@
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)} /> 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)}