From d9c0663d3ffc53cb675e1bd0b7a98b43cf459104 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Fri, 26 Jun 2026 02:38:00 +0530 Subject: [PATCH] feat(artifacts): Save-As dialog + in-progress wiring + failed-card retry (#3162) (#4127) --- app/src-tauri/Cargo.lock | 145 +++++++++++ app/src-tauri/Cargo.toml | 4 + app/src-tauri/capabilities/default.json | 1 + .../permissions/allow-artifact-save.toml | 11 + app/src-tauri/src/artifact_commands.rs | 230 +++++++++++++++--- app/src-tauri/src/lib.rs | 13 +- app/src/components/chat/ArtifactCard.test.tsx | 38 +-- app/src/components/chat/ArtifactCard.tsx | 7 +- app/src/components/chat/ChatFilesPanel.tsx | 4 + .../chat/__tests__/ArtifactCard.test.tsx | 24 +- app/src/pages/Conversations.tsx | 33 ++- app/src/providers/ChatRuntimeProvider.tsx | 16 ++ .../ChatRuntimeProvider.artifacts.test.tsx | 24 ++ .../__tests__/artifactDownloadService.test.ts | 73 ++++++ .../chatService.artifactEvents.test.ts | 91 ++++++- app/src/services/artifactDownloadService.ts | 121 ++++++--- app/src/services/chatService.ts | 88 +++++++ .../chatRuntimeSlice.artifacts.test.ts | 59 +++++ app/src/store/chatRuntimeSlice.ts | 14 ++ src/openhuman/artifacts/ops.rs | 100 ++++++++ src/openhuman/artifacts/ops_tests.rs | 102 ++++++++ src/openhuman/artifacts/schemas.rs | 103 +++++++- src/openhuman/artifacts/store.rs | 105 +++++++- src/openhuman/artifacts/store_tests.rs | 81 ++++++ src/openhuman/tools/impl/presentation/mod.rs | 19 ++ 25 files changed, 1386 insertions(+), 120 deletions(-) create mode 100644 app/src-tauri/permissions/allow-artifact-save.toml diff --git a/app/src-tauri/Cargo.lock b/app/src-tauri/Cargo.lock index 06f5b1308..8847e885e 100644 --- a/app/src-tauri/Cargo.lock +++ b/app/src-tauri/Cargo.lock @@ -30,6 +30,7 @@ dependencies = [ "rand 0.9.4", "reqwest 0.12.28", "resvg", + "rfd", "rusqlite", "rustls", "sentry", @@ -262,6 +263,27 @@ version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7c02d123df017efcdfbd739ef81735b36c5ba83ec3c59c80a9d7ecc718f92e50" +[[package]] +name = "ashpd" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d2f3f79755c74fd155000314eb349864caa787c6592eace6c6882dad873d9c39" +dependencies = [ + "enumflags2", + "futures-channel", + "futures-util", + "rand 0.9.4", + "raw-window-handle", + "serde", + "serde_repr", + "tokio", + "url", + "wayland-backend", + "wayland-client", + "wayland-protocols", + "zbus", +] + [[package]] name = "async-broadcast" version = "0.7.2" @@ -2071,6 +2093,15 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "dlib" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ab8ecd87370524b461f8557c119c405552c396ed91fc0a8eec68679eab26f94a" +dependencies = [ + "libloading 0.8.9", +] + [[package]] name = "dlopen2" version = "0.8.2" @@ -2133,6 +2164,12 @@ version = "0.15.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1aaf95b3e5c8f23aa320147307562d361db0ae0d51242340f558153b4eb2439b" +[[package]] +name = "downcast-rs" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75b325c5dbd37f80359721ad39aca5a29fb04c89279657cffdda8736d0c0b9d2" + [[package]] name = "download-cef" version = "2.3.2" @@ -6126,6 +6163,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "pollster" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f3a9f18d041e6d0e102a0a46750538147e5e8992d3b4873aaafee2520b00ce3" + [[package]] name = "poly1305" version = "0.8.0" @@ -6436,6 +6479,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "quick-xml" +version = "0.39.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdcc8dd4e2f670d309a5f0e83fe36dfdc05af317008fea29144da1a2ac858e5e" +dependencies = [ + "memchr", +] + [[package]] name = "quinn" version = "0.11.9" @@ -6885,6 +6937,30 @@ dependencies = [ "subtle", ] +[[package]] +name = "rfd" +version = "0.15.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ef2bee61e6cffa4635c72d7d81a84294e28f0930db0ddcb0f66d10244674ebed" +dependencies = [ + "ashpd", + "block2 0.6.2", + "dispatch2", + "js-sys", + "log", + "objc2 0.6.4", + "objc2-app-kit 0.3.2", + "objc2-core-foundation", + "objc2-foundation 0.3.2", + "pollster", + "raw-window-handle", + "urlencoding", + "wasm-bindgen", + "wasm-bindgen-futures", + "web-sys", + "windows-sys 0.59.0", +] + [[package]] name = "rgb" version = "0.8.53" @@ -7206,6 +7282,12 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "scoped-tls" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1cf6437eb19a8f4a6cc0f7dca544973b0b78843adbfeb3683d1a94a0024a294" + [[package]] name = "scopeguard" version = "1.2.0" @@ -8877,6 +8959,7 @@ dependencies = [ "signal-hook-registry", "socket2", "tokio-macros", + "tracing", "windows-sys 0.61.2", ] @@ -9899,6 +9982,66 @@ dependencies = [ "semver", ] +[[package]] +name = "wayland-backend" +version = "0.3.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2857dd20b54e916ec7253b3d6b4d5c4d7d4ca2c33c2e11c6c76a99bd8744755d" +dependencies = [ + "cc", + "downcast-rs", + "rustix", + "scoped-tls", + "smallvec", + "wayland-sys", +] + +[[package]] +name = "wayland-client" +version = "0.31.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "645c7c96bb74690c3189b5c9cb4ca1627062bb23693a4fad9d8c3de958260144" +dependencies = [ + "bitflags 2.11.1", + "rustix", + "wayland-backend", + "wayland-scanner", +] + +[[package]] +name = "wayland-protocols" +version = "0.32.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23d0c813de3daa2ed6520af85a3bd49b0e722a3078506899aa9686fea58dc4b6" +dependencies = [ + "bitflags 2.11.1", + "wayland-backend", + "wayland-client", + "wayland-scanner", +] + +[[package]] +name = "wayland-scanner" +version = "0.31.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9c324a910fd86ebdc364a3e61ec1f11737d3b1d6c273c0239ee8ff4bc0d24b4a" +dependencies = [ + "proc-macro2", + "quick-xml 0.39.4", + "quote", +] + +[[package]] +name = "wayland-sys" +version = "0.31.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d8eab23fefc9e41f8e841df4a9c707e8a8c4ed26e944ef69297184de2785e3be" +dependencies = [ + "dlib", + "log", + "pkg-config", +] + [[package]] name = "web-sys" version = "0.3.98" @@ -11108,6 +11251,7 @@ dependencies = [ "rustix", "serde", "serde_repr", + "tokio", "tracing", "uds_windows", "uuid 1.23.1", @@ -11358,6 +11502,7 @@ dependencies = [ "endi", "enumflags2", "serde", + "url", "winnow 1.0.2", "zvariant_derive", "zvariant_utils", diff --git a/app/src-tauri/Cargo.toml b/app/src-tauri/Cargo.toml index 1a16bac7c..8d5a1782a 100644 --- a/app/src-tauri/Cargo.toml +++ b/app/src-tauri/Cargo.toml @@ -63,6 +63,10 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" toml = "0.8" directories = "5" +# Native Save-As file dialog for artifact export (#3162). default-features +# off + xdg-portal keeps Linux off GTK (the Downloads-copy fallback covers +# headless CI where no portal is available); tokio drives the async backend. +rfd = { version = "0.15", default-features = false, features = ["xdg-portal", "tokio"] } # Used by gmail/cdp_fetch for decoding binary IO.read chunks. Base64 is # only emitted by CDP IO.read when the stream contains non-UTF-8 bytes, # but we opt into the feature to stay robust against unexpected responses. diff --git a/app/src-tauri/capabilities/default.json b/app/src-tauri/capabilities/default.json index b922e7289..c3c89b7ef 100644 --- a/app/src-tauri/capabilities/default.json +++ b/app/src-tauri/capabilities/default.json @@ -32,6 +32,7 @@ "allow-core-process", "allow-workspace-files", "allow-artifact-download", + "allow-artifact-save", "allow-app-update", "allow-loopback-oauth" ] diff --git a/app/src-tauri/permissions/allow-artifact-save.toml b/app/src-tauri/permissions/allow-artifact-save.toml new file mode 100644 index 000000000..536e226c7 --- /dev/null +++ b/app/src-tauri/permissions/allow-artifact-save.toml @@ -0,0 +1,11 @@ +[[permission]] +identifier = "allow-artifact-save" +description = "Allow exporting an agent-generated artifact to a user-chosen path via the native Save-As dialog (#3162)" + +[permission.commands] + +allow = [ + "save_artifact_via_dialog", +] + +deny = [] diff --git a/app/src-tauri/src/artifact_commands.rs b/app/src-tauri/src/artifact_commands.rs index d11dbfd89..1dbb5f2b7 100644 --- a/app/src-tauri/src/artifact_commands.rs +++ b/app/src-tauri/src/artifact_commands.rs @@ -1,30 +1,129 @@ -//! Tauri command for downloading agent-generated artifacts (#2779). +//! Tauri commands for exporting agent-generated artifacts (#2779, #3162). //! -//! Contract: the frontend resolves the artifact's absolute source -//! path via the existing `openhuman.ai_get_artifact` core RPC, then -//! invokes [`download_artifact_to_downloads`] with that source path -//! plus a filename hint. The command: +//! Two export paths, both fed by the frontend resolving an artifact's +//! absolute source path via the `openhuman.ai_get_artifact` core RPC: //! -//! 1. Validates both inputs (no path traversal in the filename, source -//! must be absolute + on disk). -//! 2. Resolves the user's Downloads directory via the `dirs` crate. -//! 3. Picks a non-colliding destination filename — `name.pptx`, -//! `name (1).pptx`, `name (2).pptx`, … -//! 4. Copies source → dest with `tokio::fs::copy`. -//! 5. Returns the absolute dest path so the frontend can show a -//! "Saved to …" toast with a "Reveal in Finder" button (the -//! `opener:allow-reveal-item-in-dir` capability is already wired). +//! 1. [`save_artifact_via_dialog`] (#3162) — opens a native Save-As +//! dialog (macOS / Windows / Linux) pre-filled with the artifact's +//! filename and copies the bytes to the user-chosen destination. +//! Returns `Ok(None)` when the user cancels. Backed by the `rfd` +//! crate, which talks to the OS dialog APIs directly and does NOT +//! pull `tauri-plugin-fs` (whose `schemars` version conflict was the +//! reason the original #2779 work shipped the Downloads fallback +//! below instead of a dialog). +//! 2. [`download_artifact_to_downloads`] (#2779) — copies the artifact +//! into the user's Downloads directory with a non-colliding name and +//! returns the dest path so the UI can offer "Reveal in Finder". +//! Retained as the fallback the frontend uses when the dialog is +//! unavailable (e.g. no portal on headless Linux) or the user cancels. +//! Cross-platform — previously macOS/Linux-only, un-gated so the +//! Save-As fallback works on Windows too. //! -//! Why Downloads instead of a native save-file dialog: the -//! `tauri-plugin-dialog` crate pulls `tauri-plugin-fs` transitively, -//! which currently breaks the openhuman build with a `schemars` -//! version conflict. The Downloads + reveal pattern satisfies the -//! "user-chosen destination" intent of issue #2779 AC#3 without -//! widening the Tauri allow-list, and matches what most desktop chat -//! apps do for downloaded attachments. +//! Both validate that the source is an existing file inside the +//! OpenHuman data dir's `artifacts/` tree, and sanitize the filename +//! hint, so the renderer can never copy an arbitrary local file out nor +//! write outside the chosen directory. use std::path::{Path, PathBuf}; +/// Open a native Save-As dialog pre-filled with `suggested_filename` and +/// copy the artifact at `source_path` to the chosen destination (#3162). +/// +/// Returns: +/// - `Ok(Some(dest))` — the absolute path the user saved to. +/// - `Ok(None)` — the user dismissed the dialog (not an error; the +/// frontend simply stops). +/// - `Err(_)` — bad inputs or a copy failure; the frontend falls back to +/// [`download_artifact_to_downloads`] where available. +#[tauri::command] +pub async fn save_artifact_via_dialog( + source_path: String, + suggested_filename: String, +) -> Result, String> { + let source = validate_source(&source_path)?; + let sanitized = sanitize_filename(&suggested_filename)?; + + // `rfd` drives the OS-native dialog. On Linux this is the xdg-desktop + // portal (no GTK link); on macOS/Windows the system panel. The await + // resolves when the user picks a path or cancels. + let handle = rfd::AsyncFileDialog::new() + .set_file_name(&sanitized) + .save_file() + .await; + + let Some(file) = handle else { + log::info!("[artifact_commands] save_artifact_via_dialog cancelled by user"); + return Ok(None); + }; + + let dest = file.path().to_path_buf(); + let bytes = copy_to_path(&source, &dest).await?; + log::info!( + "[artifact_commands] save_artifact_via_dialog bytes={bytes} dest={}", + dest.display() + ); + Ok(Some(dest.display().to_string())) +} + +/// Validate a renderer-supplied source path: must be a non-empty, +/// absolute path that exists on disk AND resolve inside the OpenHuman +/// data directory's `artifacts/` tree. The path always originates from +/// the core `ai_get_artifact` RPC's `absolute_path`, but the command is +/// reachable by the renderer directly, so we re-validate the trust +/// boundary here — without the artifacts-root check a compromised +/// renderer could copy any readable local file out through the Save-As +/// dialog under an artifact-looking name (Codex P2). +fn validate_source(source_path: &str) -> Result { + if source_path.trim().is_empty() { + return Err("source_path must not be empty".to_string()); + } + let source = PathBuf::from(source_path); + if !source.is_absolute() { + return Err(format!( + "source_path must be absolute (came from ai_get_artifact): {source_path:?}" + )); + } + if !source.is_file() { + return Err(format!( + "artifact source not present on disk: {source_path}" + )); + } + let root = crate::cef_profile::default_root_openhuman_dir()?; + assert_artifact_source(&source, &root)?; + Ok(source) +} + +/// Confirm `source` resolves inside `root` (the OpenHuman data dir) and +/// carries an `artifacts` path component — i.e. it is a workspace +/// artifact, not an arbitrary local file. Canonicalizes both sides so +/// symlink trickery can't escape the root. Isolated for unit testing +/// without touching the real home directory. +fn assert_artifact_source(source: &Path, root: &Path) -> Result<(), String> { + let canon_source = source + .canonicalize() + .map_err(|e| format!("cannot resolve source path: {e}"))?; + let canon_root = root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); + if !canon_source.starts_with(&canon_root) { + return Err("source must be inside the OpenHuman data directory".to_string()); + } + if !canon_source + .components() + .any(|c| c.as_os_str() == "artifacts") + { + return Err("source must be a workspace artifact file".to_string()); + } + Ok(()) +} + +/// Copy `source` to `dest`, returning the byte count. Shared by the +/// Save-As dialog flow; isolated so it is unit-testable without driving +/// a real OS dialog. +async fn copy_to_path(source: &Path, dest: &Path) -> Result { + tokio::fs::copy(source, dest) + .await + .map_err(|e| format!("failed to copy artifact to {:?}: {e}", dest)) +} + /// Maximum number of `(N)` suffixes we'll append when picking a /// non-colliding filename. After 1000 we give up and append a UUID /// suffix instead so the download never silently overwrites. @@ -35,23 +134,10 @@ pub async fn download_artifact_to_downloads( source_path: String, filename: String, ) -> Result { - if source_path.trim().is_empty() { - return Err("source_path must not be empty".to_string()); - } + let source = validate_source(&source_path)?; if filename.trim().is_empty() { return Err("filename must not be empty".to_string()); } - let source = PathBuf::from(&source_path); - if !source.is_absolute() { - return Err(format!( - "source_path must be absolute (came from ai_get_artifact): {source_path:?}" - )); - } - if !source.is_file() { - return Err(format!( - "artifact source not present on disk: {source_path}" - )); - } let sanitized = sanitize_filename(&filename)?; let downloads = directories::UserDirs::new() @@ -62,9 +148,7 @@ pub async fn download_artifact_to_downloads( .map_err(|e| format!("failed to ensure Downloads dir {:?}: {e}", downloads))?; let dest = pick_unique_path(&downloads, &sanitized); - let bytes = tokio::fs::copy(&source, &dest) - .await - .map_err(|e| format!("failed to copy artifact to {:?}: {e}", dest))?; + let bytes = copy_to_path(&source, &dest).await?; log::info!( "[artifact_commands] download_artifact_to_downloads bytes={bytes} dest={}", @@ -76,7 +160,7 @@ pub async fn download_artifact_to_downloads( /// Strip path-traversal characters from a filename hint. The /// renderer is expected to pass something like `"My Deck.pptx"`; /// reject anything that contains a separator or null byte so a -/// malicious `ai_get_artifact` response can never escape Downloads. +/// malicious `ai_get_artifact` response can never escape the chosen dir. fn sanitize_filename(name: &str) -> Result { let trimmed = name.trim(); if trimmed.is_empty() { @@ -166,6 +250,74 @@ mod tests { assert_eq!(sanitize_filename(" trim me ").unwrap(), "trim me"); } + #[test] + fn validate_source_rejects_relative_and_empty() { + assert!(validate_source("").is_err()); + assert!(validate_source("relative/path.pptx").is_err()); + assert!(validate_source("/definitely/not/here.pptx").is_err()); + } + + #[test] + fn assert_artifact_source_accepts_file_under_artifacts_root() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + let art = root.join("users/u1/workspace/artifacts/a-1"); + std::fs::create_dir_all(&art).unwrap(); + let file = art.join("deck.pptx"); + std::fs::write(&file, b"x").unwrap(); + assert!(assert_artifact_source(&file, root).is_ok()); + } + + #[test] + fn assert_artifact_source_rejects_file_without_artifacts_component() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + let other = root.join("users/u1/secrets"); + std::fs::create_dir_all(&other).unwrap(); + let file = other.join("token.txt"); + std::fs::write(&file, b"x").unwrap(); + assert!(assert_artifact_source(&file, root).is_err()); + } + + #[test] + fn assert_artifact_source_rejects_file_outside_root() { + let root_dir = tempfile::tempdir().unwrap(); + let outside_dir = tempfile::tempdir().unwrap(); + // Even with an `artifacts` segment, a path outside the root is denied. + let art = outside_dir.path().join("artifacts"); + std::fs::create_dir_all(&art).unwrap(); + let file = art.join("evil.pptx"); + std::fs::write(&file, b"x").unwrap(); + assert!(assert_artifact_source(&file, root_dir.path()).is_err()); + } + + #[tokio::test] + async fn copy_to_path_copies_bytes() { + let temp = tempfile::tempdir().unwrap(); + let src = temp.path().join("src.pptx"); + let dst = temp.path().join("dst.pptx"); + std::fs::write(&src, b"deck-bytes").unwrap(); + let n = copy_to_path(&src, &dst).await.unwrap(); + assert_eq!(n, b"deck-bytes".len() as u64); + assert_eq!(std::fs::read(&dst).unwrap(), b"deck-bytes"); + } + + #[tokio::test] + async fn save_via_dialog_rejects_bad_source() { + // Validation runs before any dialog is shown, so these resolve + // without user interaction. + assert!( + save_artifact_via_dialog(String::new(), "x.pptx".to_string()) + .await + .is_err() + ); + assert!( + save_artifact_via_dialog("relative".to_string(), "x.pptx".to_string()) + .await + .is_err() + ); + } + #[test] fn split_stem_ext_pairs() { assert_eq!( diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 2b41610f5..16e11c1bc 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -4,7 +4,8 @@ compile_error!("src-tauri host supports desktop (Windows/macOS/Linux) only. Mobile lives in app/src-tauri-mobile."); mod app_update; -#[cfg(any(target_os = "macos", target_os = "linux"))] +// Artifact export commands (#2779, #3162) — both cross-platform +// (macOS/Windows/Linux): native Save-As dialog (rfd) + Downloads copy. mod artifact_commands; mod cdp; // macOS/Linux only: depends on the `nix` crate (a `cfg(unix)` dependency) and @@ -3611,11 +3612,11 @@ pub fn run() { core_rpc::relay_http_rpc, overlay_parent_rpc_url, process_diagnostics_list_owned, - // `mod artifact_commands;` is `#[cfg(any(target_os = "macos", target_os = "linux"))]` - // (Downloads-dir + `tokio::fs::copy` flow is non-Windows-only today). - // The handler entry MUST carry the same gate or Windows builds fail - // with "function not found in scope" (CR #3328947313 on PR #3026). - #[cfg(any(target_os = "macos", target_os = "linux"))] + // Artifact export commands — both cross-platform (#3162). The + // Downloads command was previously macOS/Linux-gated, but the + // `directories` + `tokio::fs::copy` flow compiles on Windows too, + // and the Save-As fallback needs it there (CodeRabbit on #4127). + artifact_commands::save_artifact_via_dialog, artifact_commands::download_artifact_to_downloads, check_core_update, apply_core_update, diff --git a/app/src/components/chat/ArtifactCard.test.tsx b/app/src/components/chat/ArtifactCard.test.tsx index 61a21ebef..bb9abe8a2 100644 --- a/app/src/components/chat/ArtifactCard.test.tsx +++ b/app/src/components/chat/ArtifactCard.test.tsx @@ -6,10 +6,10 @@ import ArtifactCard from './ArtifactCard'; // Mock the artifact download service — the card only consumes the // two public functions, so a per-test override is enough. -const downloadArtifactMock = vi.fn(); +const saveArtifactViaDialogMock = vi.fn(); const revealArtifactInFileManagerMock = vi.fn(); vi.mock('../../services/artifactDownloadService', () => ({ - downloadArtifact: (...args: unknown[]) => downloadArtifactMock(...args), + saveArtifactViaDialog: (...args: unknown[]) => saveArtifactViaDialogMock(...args), revealArtifactInFileManager: (...args: unknown[]) => revealArtifactInFileManagerMock(...args), })); @@ -27,7 +27,7 @@ function snapshot(overrides: Partial = {}): ArtifactSnapshot { } beforeEach(() => { - downloadArtifactMock.mockReset(); + saveArtifactViaDialogMock.mockReset(); revealArtifactInFileManagerMock.mockReset(); }); @@ -82,7 +82,7 @@ describe(' — ready state', () => { }); it('drives the download → done flow and reveals on click', async () => { - downloadArtifactMock.mockResolvedValueOnce({ + saveArtifactViaDialogMock.mockResolvedValueOnce({ ok: true, path: '/Users/me/Downloads/Quarterly Deck.pptx', }); @@ -94,8 +94,8 @@ describe(' — ready state', () => { // Service called with (id, title, ext) where ext comes from the // title's extension when present, or kind fallback otherwise. - await waitFor(() => expect(downloadArtifactMock).toHaveBeenCalledTimes(1)); - expect(downloadArtifactMock).toHaveBeenCalledWith('a-1', 'Quarterly Deck', 'pptx'); + await waitFor(() => expect(saveArtifactViaDialogMock).toHaveBeenCalledTimes(1)); + expect(saveArtifactViaDialogMock).toHaveBeenCalledWith('a-1', 'Quarterly Deck', 'pptx'); // Saved-to row appears with the reveal button. await screen.findByText( @@ -113,40 +113,40 @@ describe(' — ready state', () => { }); it('falls back to the kind-based extension when title has no dot', async () => { - downloadArtifactMock.mockResolvedValueOnce({ ok: true, path: '/tmp/Doc.pdf' }); + saveArtifactViaDialogMock.mockResolvedValueOnce({ ok: true, path: '/tmp/Doc.pdf' }); render( ); fireEvent.click(screen.getByRole('button', { name: 'Download' })); - await waitFor(() => expect(downloadArtifactMock).toHaveBeenCalledTimes(1)); - expect(downloadArtifactMock).toHaveBeenCalledWith('a-2', 'Doc', 'pdf'); + await waitFor(() => expect(saveArtifactViaDialogMock).toHaveBeenCalledTimes(1)); + expect(saveArtifactViaDialogMock).toHaveBeenCalledWith('a-2', 'Doc', 'pdf'); }); it('falls back to png for image kind without an extension', async () => { - downloadArtifactMock.mockResolvedValueOnce({ ok: true, path: '/tmp/Pic.png' }); + saveArtifactViaDialogMock.mockResolvedValueOnce({ ok: true, path: '/tmp/Pic.png' }); render( ); fireEvent.click(screen.getByRole('button', { name: 'Download' })); - await waitFor(() => expect(downloadArtifactMock).toHaveBeenCalledTimes(1)); - expect(downloadArtifactMock).toHaveBeenCalledWith('a-3', 'Pic', 'png'); + await waitFor(() => expect(saveArtifactViaDialogMock).toHaveBeenCalledTimes(1)); + expect(saveArtifactViaDialogMock).toHaveBeenCalledWith('a-3', 'Pic', 'png'); }); it('falls back to bin for the "other" kind without an extension', async () => { - downloadArtifactMock.mockResolvedValueOnce({ ok: true, path: '/tmp/Blob.bin' }); + saveArtifactViaDialogMock.mockResolvedValueOnce({ ok: true, path: '/tmp/Blob.bin' }); render( ); fireEvent.click(screen.getByRole('button', { name: 'Download' })); - await waitFor(() => expect(downloadArtifactMock).toHaveBeenCalledTimes(1)); - expect(downloadArtifactMock).toHaveBeenCalledWith('a-4', 'Blob', 'bin'); + await waitFor(() => expect(saveArtifactViaDialogMock).toHaveBeenCalledTimes(1)); + expect(saveArtifactViaDialogMock).toHaveBeenCalledWith('a-4', 'Blob', 'bin'); }); it('uses the existing extension on the title when present', async () => { - downloadArtifactMock.mockResolvedValueOnce({ ok: true, path: '/tmp/notes.txt' }); + saveArtifactViaDialogMock.mockResolvedValueOnce({ ok: true, path: '/tmp/notes.txt' }); render( — ready state', () => { ); fireEvent.click(screen.getByRole('button', { name: 'Download' })); - await waitFor(() => expect(downloadArtifactMock).toHaveBeenCalledTimes(1)); - expect(downloadArtifactMock).toHaveBeenCalledWith('a-5', 'notes.txt', 'txt'); + await waitFor(() => expect(saveArtifactViaDialogMock).toHaveBeenCalledTimes(1)); + expect(saveArtifactViaDialogMock).toHaveBeenCalledWith('a-5', 'notes.txt', 'txt'); }); it('surfaces the service error message when download fails', async () => { - downloadArtifactMock.mockResolvedValueOnce({ ok: false, error: 'disk full' }); + saveArtifactViaDialogMock.mockResolvedValueOnce({ ok: false, error: 'disk full' }); render(); fireEvent.click(screen.getByRole('button', { name: 'Download' })); diff --git a/app/src/components/chat/ArtifactCard.tsx b/app/src/components/chat/ArtifactCard.tsx index d33eeb3ed..687aa245e 100644 --- a/app/src/components/chat/ArtifactCard.tsx +++ b/app/src/components/chat/ArtifactCard.tsx @@ -3,8 +3,8 @@ import { useState } from 'react'; import { formatFileSize } from '../../lib/attachments'; import { useT } from '../../lib/i18n/I18nContext'; import { - downloadArtifact, revealArtifactInFileManager, + saveArtifactViaDialog, } from '../../services/artifactDownloadService'; import type { ArtifactSnapshot } from '../../store/chatRuntimeSlice'; @@ -170,9 +170,12 @@ export default function ArtifactCard({ artifact, onRetry }: ArtifactCardProps) { const handleDownload = async () => { setDownload({ state: 'downloading' }); const ext = extensionFor(artifact.kind, artifact.title); - const outcome = await downloadArtifact(artifact.artifactId, artifact.title, ext); + const outcome = await saveArtifactViaDialog(artifact.artifactId, artifact.title, ext); if (outcome.ok) { setDownload({ state: 'done', path: outcome.path }); + } else if (outcome.code === 'CANCELLED') { + // User dismissed the Save-As dialog — quietly return to idle. + setDownload({ state: 'idle' }); } else { setDownload({ state: 'error', error: outcome.error }); } diff --git a/app/src/components/chat/ChatFilesPanel.tsx b/app/src/components/chat/ChatFilesPanel.tsx index d0d279a16..119f1dcbc 100644 --- a/app/src/components/chat/ChatFilesPanel.tsx +++ b/app/src/components/chat/ChatFilesPanel.tsx @@ -63,6 +63,10 @@ function localizeErrorCode( return t('chat.files.error.download_failed'); case 'DELETE_FAILED': return t('chat.files.error.delete_failed'); + case 'CANCELLED': + // User-initiated dialog dismissal — not a real error. Callers treat + // it as a no-op; surface nothing (fall back to raw text if passed). + return fallback ?? ''; default: { // Exhaustive guard: a new code added to ArtifactErrorCode without a // matching arm here will fail to type-check. diff --git a/app/src/components/chat/__tests__/ArtifactCard.test.tsx b/app/src/components/chat/__tests__/ArtifactCard.test.tsx index a4d5c0979..c0f6b471a 100644 --- a/app/src/components/chat/__tests__/ArtifactCard.test.tsx +++ b/app/src/components/chat/__tests__/ArtifactCard.test.tsx @@ -2,14 +2,14 @@ import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { - downloadArtifact, revealArtifactInFileManager, + saveArtifactViaDialog, } from '../../../services/artifactDownloadService'; import type { ArtifactSnapshot } from '../../../store/chatRuntimeSlice'; import ArtifactCard from '../ArtifactCard'; vi.mock('../../../services/artifactDownloadService', () => ({ - downloadArtifact: vi.fn(), + saveArtifactViaDialog: vi.fn(), revealArtifactInFileManager: vi.fn(), })); @@ -75,15 +75,15 @@ describe('ArtifactCard', () => { expect(screen.getByRole('button', { name: 'Download' })).toBeInTheDocument(); }); - it('on Download click → calls downloadArtifact with title-derived extension on success', async () => { - vi.mocked(downloadArtifact).mockResolvedValueOnce({ + it('on Download click → calls saveArtifactViaDialog with title-derived extension on success', async () => { + vi.mocked(saveArtifactViaDialog).mockResolvedValueOnce({ ok: true, path: '/Users/me/Downloads/Climate Deck.pptx', }); render(); fireEvent.click(screen.getByRole('button', { name: 'Download' })); await waitFor(() => { - expect(downloadArtifact).toHaveBeenCalledWith('art-1', 'climate-deck.pptx', 'pptx'); + expect(saveArtifactViaDialog).toHaveBeenCalledWith('art-1', 'climate-deck.pptx', 'pptx'); }); // Saved-to label appears with the resolved path await waitFor(() => { @@ -95,7 +95,7 @@ describe('ArtifactCard', () => { }); it('on Reveal click → calls revealArtifactInFileManager with the saved path', async () => { - vi.mocked(downloadArtifact).mockResolvedValueOnce({ + vi.mocked(saveArtifactViaDialog).mockResolvedValueOnce({ ok: true, path: '/Users/me/Downloads/Climate Deck.pptx', }); @@ -113,7 +113,7 @@ describe('ArtifactCard', () => { }); it('on Download failure → surfaces the error reason and leaves the Download button in place', async () => { - vi.mocked(downloadArtifact).mockResolvedValueOnce({ + vi.mocked(saveArtifactViaDialog).mockResolvedValueOnce({ ok: false, code: 'NOT_DESKTOP', error: 'Downloads are only available in the desktop app', @@ -137,27 +137,27 @@ describe('ArtifactCard', () => { ])( 'falls back to per-kind extension when title lacks one (kind=%s → ext=%s)', async (kind, expectedExt) => { - vi.mocked(downloadArtifact).mockResolvedValueOnce({ ok: true, path: '/d/x' }); + vi.mocked(saveArtifactViaDialog).mockResolvedValueOnce({ ok: true, path: '/d/x' }); render(); fireEvent.click(screen.getByRole('button', { name: 'Download' })); await waitFor(() => { - expect(downloadArtifact).toHaveBeenCalledWith('art-1', 'no-extension', expectedExt); + expect(saveArtifactViaDialog).toHaveBeenCalledWith('art-1', 'no-extension', expectedExt); }); } ); it('treats a trailing-dot title as having no extension (falls through to kind default)', async () => { - vi.mocked(downloadArtifact).mockResolvedValueOnce({ ok: true, path: '/d/x' }); + vi.mocked(saveArtifactViaDialog).mockResolvedValueOnce({ ok: true, path: '/d/x' }); render(); fireEvent.click(screen.getByRole('button', { name: 'Download' })); await waitFor(() => { - expect(downloadArtifact).toHaveBeenCalledWith('art-1', 'trailing.', 'pptx'); + expect(saveArtifactViaDialog).toHaveBeenCalledWith('art-1', 'trailing.', 'pptx'); }); }); it('Download button is disabled while a download is in flight', async () => { let resolveDownload: (v: { ok: true; path: string }) => void = () => {}; - vi.mocked(downloadArtifact).mockImplementationOnce( + vi.mocked(saveArtifactViaDialog).mockImplementationOnce( () => new Promise(r => { resolveDownload = r; diff --git a/app/src/pages/Conversations.tsx b/app/src/pages/Conversations.tsx index 3c2434cd5..9e3b7bcb0 100644 --- a/app/src/pages/Conversations.tsx +++ b/app/src/pages/Conversations.tsx @@ -36,7 +36,13 @@ import { trackEvent } from '../services/analytics'; import { applyOpenRouterFreeModels } from '../services/api/openrouterFreeModels'; import { subagentApi } from '../services/api/subagentApi'; import { threadApi } from '../services/api/threadApi'; -import { chatCancel, chatClearQueue, chatSend, useRustChat } from '../services/chatService'; +import { + aiRegenerate, + chatCancel, + chatClearQueue, + chatSend, + useRustChat, +} from '../services/chatService'; import { callCoreRpc } from '../services/coreRpcClient'; import { loadAgentProfiles, @@ -2670,12 +2676,11 @@ const Conversations = ({ // chat scroll area isn't permanently occupied — restored decks // are listable from the chip on demand. // - // NOTE: `onRetry` is intentionally omitted on `ArtifactCard` - // below — real retry (either `removeArtifact(thread, id)` to - // let the user re-prompt, or full re-dispatch of the producing - // tool call) is tracked in follow-up issue #3162. The - // failed-card UI still surfaces the truncated error reason; - // the button just stays hidden until #3162 lands. + // The failed-card Retry button re-dispatches the producing tool + // via `ai_regenerate` (#3162): the core reloads the persisted + // creation args and re-runs generation under the original + // artifact id, so the card swaps back to a spinner in place and + // then to ready/failed via the socket events. const artifactThreadId = selectedThreadId ?? firstActiveThreadId; const all = artifactThreadId ? (artifactsByThread[artifactThreadId] ?? []) : []; const live = all.filter(a => a.status !== 'ready'); @@ -2683,7 +2688,19 @@ const Conversations = ({ return (
{live.map(artifact => ( - + { + void aiRegenerate(id, artifactThreadId).catch(err => { + console.warn('[artifact] regenerate failed:', err); + }); + } + : undefined + } + /> ))}
); diff --git a/app/src/providers/ChatRuntimeProvider.tsx b/app/src/providers/ChatRuntimeProvider.tsx index cae521df3..f206b3686 100644 --- a/app/src/providers/ChatRuntimeProvider.tsx +++ b/app/src/providers/ChatRuntimeProvider.tsx @@ -48,6 +48,7 @@ import { type ToolTimelineEntry, type ToolTimelineEntryStatus, upsertArtifactFailedForThread, + upsertArtifactInProgressForThread, upsertArtifactReadyForThread, } from '../store/chatRuntimeSlice'; import { useAppDispatch, useAppSelector } from '../store/hooks'; @@ -964,6 +965,21 @@ const ChatRuntimeProvider = ({ children }: { children: React.ReactNode }) => { } }); }, + onArtifactPending: event => { + rtLog('artifact_pending', { + thread: event.thread_id, + artifact_id: event.artifact_id, + kind: event.kind, + }); + dispatch( + upsertArtifactInProgressForThread({ + threadId: event.thread_id, + artifactId: event.artifact_id, + kind: event.kind, + title: event.title, + }) + ); + }, onArtifactReady: event => { rtLog('artifact_ready', { thread: event.thread_id, diff --git a/app/src/providers/__tests__/ChatRuntimeProvider.artifacts.test.tsx b/app/src/providers/__tests__/ChatRuntimeProvider.artifacts.test.tsx index 5667366e9..1633832a9 100644 --- a/app/src/providers/__tests__/ChatRuntimeProvider.artifacts.test.tsx +++ b/app/src/providers/__tests__/ChatRuntimeProvider.artifacts.test.tsx @@ -98,6 +98,30 @@ describe('ChatRuntimeProvider — artifact event dispatch (#2779)', () => { }); }); + it('onArtifactPending upserts an in_progress snapshot keyed on the artifact id (#3162)', () => { + const listeners = renderProvider(); + + act(() => { + listeners.onArtifactPending?.({ + thread_id: 'thread-1', + artifact_id: 'a-1', + kind: 'presentation', + title: 'Deck', + workspace_dir: '/workspace', + path: 'a-1/deck.pptx', + }); + }); + + const bucket = store.getState().chatRuntime.artifactsByThread['thread-1']; + expect(bucket).toHaveLength(1); + expect(bucket?.[0]).toMatchObject({ + artifactId: 'a-1', + kind: 'presentation', + title: 'Deck', + status: 'in_progress', + }); + }); + it('onArtifactFailed records the producer-supplied error', () => { const listeners = renderProvider(); diff --git a/app/src/services/__tests__/artifactDownloadService.test.ts b/app/src/services/__tests__/artifactDownloadService.test.ts index ef1067049..25143e7ac 100644 --- a/app/src/services/__tests__/artifactDownloadService.test.ts +++ b/app/src/services/__tests__/artifactDownloadService.test.ts @@ -16,6 +16,7 @@ import { deleteArtifact, downloadArtifact, revealArtifactInFileManager, + saveArtifactViaDialog, } from '../artifactDownloadService'; import { callCoreRpc } from '../coreRpcClient'; @@ -297,3 +298,75 @@ describe('revealArtifactInFileManager', () => { warn.mockRestore(); }); }); + +describe('saveArtifactViaDialog (#3162)', () => { + beforeEach(() => { + vi.clearAllMocks(); + hoisted.isTauri.mockReturnValue(true); + }); + + const resolveOk = () => + vi + .mocked(callCoreRpc) + .mockResolvedValueOnce({ + absolute_path: '/ws/artifacts/a-1/deck.pptx', + meta: { id: 'a-1', title: 'Deck' }, + } as never); + + it('returns NOT_DESKTOP outside Tauri', async () => { + hoisted.isTauri.mockReturnValueOnce(false); + const outcome = await saveArtifactViaDialog('a-1', 'Deck', 'pptx'); + expect(outcome).toEqual({ + ok: false, + code: 'NOT_DESKTOP', + error: expect.stringContaining('desktop'), + }); + expect(callCoreRpc).not.toHaveBeenCalled(); + }); + + it('saves to the user-chosen path and returns it', async () => { + resolveOk(); + hoisted.invoke.mockResolvedValueOnce('/Users/me/Desktop/Deck.pptx'); + const outcome = await saveArtifactViaDialog('a-1', 'Deck', 'pptx'); + expect(outcome).toEqual({ ok: true, path: '/Users/me/Desktop/Deck.pptx' }); + expect(hoisted.invoke).toHaveBeenCalledWith('save_artifact_via_dialog', { + sourcePath: '/ws/artifacts/a-1/deck.pptx', + suggestedFilename: 'Deck.pptx', + }); + }); + + it('treats a null result (dialog dismissed) as CANCELLED, not an error', async () => { + resolveOk(); + hoisted.invoke.mockResolvedValueOnce(null); + const outcome = await saveArtifactViaDialog('a-1', 'Deck', 'pptx'); + expect(outcome).toEqual({ ok: false, code: 'CANCELLED', error: expect.any(String) }); + }); + + it('falls back to the Downloads copy when the dialog is unavailable', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + // First resolve (dialog path) → invoke throws; fallback re-resolves + // then invokes the Downloads command successfully. + resolveOk(); + hoisted.invoke.mockRejectedValueOnce(new Error('no portal')); + vi.mocked(callCoreRpc).mockResolvedValueOnce({ + absolute_path: '/ws/artifacts/a-1/deck.pptx', + meta: { id: 'a-1', title: 'Deck' }, + } as never); + hoisted.invoke.mockResolvedValueOnce('/Users/me/Downloads/Deck.pptx'); + + const outcome = await saveArtifactViaDialog('a-1', 'Deck', 'pptx'); + expect(outcome).toEqual({ ok: true, path: '/Users/me/Downloads/Deck.pptx' }); + expect(hoisted.invoke).toHaveBeenNthCalledWith(2, 'download_artifact_to_downloads', { + sourcePath: '/ws/artifacts/a-1/deck.pptx', + filename: 'Deck.pptx', + }); + warn.mockRestore(); + }); + + it('propagates resolve failures without showing a dialog', async () => { + vi.mocked(callCoreRpc).mockRejectedValueOnce(new Error('rpc down')); + const outcome = await saveArtifactViaDialog('a-1', 'Deck', 'pptx'); + expect(outcome).toEqual({ ok: false, code: 'RESOLVE_FAILED', error: 'rpc down' }); + expect(hoisted.invoke).not.toHaveBeenCalled(); + }); +}); diff --git a/app/src/services/__tests__/chatService.artifactEvents.test.ts b/app/src/services/__tests__/chatService.artifactEvents.test.ts index d44f91d2d..b14fb9a79 100644 --- a/app/src/services/__tests__/chatService.artifactEvents.test.ts +++ b/app/src/services/__tests__/chatService.artifactEvents.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { subscribeChatEvents } from '../chatService'; +import { aiRegenerate, subscribeChatEvents } from '../chatService'; +import { callCoreRpc } from '../coreRpcClient'; import { socketService } from '../socketService'; vi.mock('../socketService', () => ({ socketService: { getSocket: vi.fn() } })); @@ -267,3 +268,91 @@ describe('chatService — artifact_ready / artifact_failed handlers (#2779)', () expect(offEvents).toEqual(['artifact_ready', 'artifact_failed']); }); }); + +describe('chatService — artifact_pending handler (#3162)', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('subscribes to artifact_pending under its canonical snake_case name', () => { + const socket = createMockSocket(); + vi.mocked(socketService.getSocket).mockReturnValue(socket as never); + + subscribeChatEvents({ onArtifactPending: () => {} }); + + const events = socket.on.mock.calls.map(call => call[0]); + expect(events).toEqual(['artifact_pending']); + }); + + it('flattens the wire envelope into a typed ArtifactPendingEvent', () => { + const socket = createMockSocket(); + vi.mocked(socketService.getSocket).mockReturnValue(socket as never); + const onArtifactPending = vi.fn(); + + subscribeChatEvents({ onArtifactPending }); + + socket.emit('artifact_pending', { + thread_id: 'thread-1', + client_id: 'web-x', + args: { + artifact_id: 'a-1', + kind: 'presentation', + title: 'Deck', + workspace_dir: '/workspace', + path: 'a-1/deck.pptx', + }, + }); + + expect(onArtifactPending).toHaveBeenCalledTimes(1); + expect(onArtifactPending.mock.calls[0]![0]).toEqual({ + thread_id: 'thread-1', + client_id: 'web-x', + artifact_id: 'a-1', + kind: 'presentation', + title: 'Deck', + workspace_dir: '/workspace', + path: 'a-1/deck.pptx', + }); + }); + + it('drops a pending payload missing required args', () => { + const socket = createMockSocket(); + vi.mocked(socketService.getSocket).mockReturnValue(socket as never); + const onArtifactPending = vi.fn(); + + subscribeChatEvents({ onArtifactPending }); + + // Missing `path` → malformed, skipped. + socket.emit('artifact_pending', { + thread_id: 'thread-1', + args: { artifact_id: 'a-1', kind: 'presentation', title: 'Deck', workspace_dir: '/ws' }, + }); + + expect(onArtifactPending).not.toHaveBeenCalled(); + }); +}); + +describe('aiRegenerate (#3162)', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('calls ai_regenerate with the socket client_id for routing', async () => { + vi.mocked(socketService.getSocket).mockReturnValue({ id: 'socket-9' } as never); + vi.mocked(callCoreRpc).mockResolvedValueOnce({} as never); + + const ok = await aiRegenerate('a-1', 'thread-1'); + + expect(ok).toBe(true); + expect(callCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.ai_regenerate', + params: { artifact_id: 'a-1', thread_id: 'thread-1', client_id: 'socket-9' }, + }); + }); + + it('throws when the socket is not connected', async () => { + vi.mocked(socketService.getSocket).mockReturnValue(undefined as never); + await expect(aiRegenerate('a-1', 'thread-1')).rejects.toThrow('Socket not connected'); + expect(callCoreRpc).not.toHaveBeenCalled(); + }); +}); diff --git a/app/src/services/artifactDownloadService.ts b/app/src/services/artifactDownloadService.ts index 9beed937e..538518c2c 100644 --- a/app/src/services/artifactDownloadService.ts +++ b/app/src/services/artifactDownloadService.ts @@ -1,20 +1,20 @@ /** - * Artifact download service (#2779). + * Artifact export service (#2779, #3162). * - * Flow: + * All paths first resolve the artifact's absolute on-disk path + meta + * via the `openhuman.ai_get_artifact` core RPC, then hand a source path + * + filename hint to a Tauri command: * - * 1. Call `openhuman.ai_get_artifact` via the existing core RPC client - * to resolve the artifact's absolute on-disk path + meta. - * 2. Invoke the Tauri `download_artifact_to_downloads` command with - * the source path + a filename hint built from the artifact's - * title. The command picks a non-colliding name under the user's - * Downloads directory and copies the file. - * 3. Return the resolved dest path so the UI can show a "Saved to …" - * toast with a "Reveal in Finder" button (the `opener` plugin's - * `reveal-item-in-dir` capability is already wired). + * - {@link saveArtifactViaDialog} (#3162) — native Save-As dialog + * pre-filled with the filename; user picks the destination. Cancel is + * reported as `{ ok: false, code: 'CANCELLED' }`. Falls back to the + * Downloads copy if the dialog itself is unavailable. + * - {@link downloadArtifact} (#2779) — copies into the user's Downloads + * directory with a non-colliding name and returns the dest path so the + * UI can offer "Reveal in Finder". * - * No-ops outside Tauri (browser dev preview) — the download flow only - * makes sense in the desktop shell. + * No-ops outside Tauri (browser dev preview) — export only makes sense in + * the desktop shell. */ import { revealItemInDir } from '@tauri-apps/plugin-opener'; @@ -35,6 +35,7 @@ export type ArtifactErrorCode = | 'MISSING_ARTIFACT_PATH' | 'RESOLVE_FAILED' | 'DOWNLOAD_FAILED' + | 'CANCELLED' | 'DELETE_FAILED'; /** Outcome surfaced to the UI for a single download attempt. */ @@ -80,25 +81,24 @@ interface AiGetArtifactData { meta?: { id?: string; title?: string; path?: string; kind?: string; status?: string }; } +/** Resolved source path + filename hint for an artifact export. */ +interface ResolvedExport { + ok: true; + sourcePath: string; + filename: string; +} +type ResolveExportResult = ResolvedExport | { ok: false; code: ArtifactErrorCode; error: string }; + /** - * Resolve the source path + filename hint, then copy to Downloads. - * - * `extension` is the file extension WITHOUT the leading dot - * (`"pptx"`, `"pdf"`, …). Used to build the Downloads filename when - * the title doesn't already carry one. + * Resolve an artifact's absolute on-disk path (via `ai_get_artifact`) + * and build the suggested filename. Shared by the Save-As and Downloads + * export paths so both apply identical title/extension handling. */ -export async function downloadArtifact( +async function resolveArtifactForExport( artifactId: string, fallbackTitle: string, extension: string -): Promise { - if (!isTauri()) { - return { - ok: false, - code: 'NOT_DESKTOP', - error: 'Downloads are only available in the desktop app', - }; - } +): Promise { if (!artifactId.trim()) { return { ok: false, code: 'MISSING_ARTIFACT_ID', error: 'artifact id missing' }; } @@ -136,8 +136,32 @@ export async function downloadArtifact( const titleHasSameExt = ext.length > 0 && title.toLowerCase().endsWith(`.${ext.toLowerCase()}`); const filename = ext && !titleHasExtension && !titleHasSameExt ? `${title}.${ext}` : title; + return { ok: true, sourcePath, filename }; +} + +export async function downloadArtifact( + artifactId: string, + fallbackTitle: string, + extension: string +): Promise { + if (!isTauri()) { + return { + ok: false, + code: 'NOT_DESKTOP', + error: 'Downloads are only available in the desktop app', + }; + } + + const resolved = await resolveArtifactForExport(artifactId, fallbackTitle, extension); + if (!resolved.ok) { + return { ok: false, code: resolved.code, error: resolved.error }; + } + try { - const dest = await invoke('download_artifact_to_downloads', { sourcePath, filename }); + const dest = await invoke('download_artifact_to_downloads', { + sourcePath: resolved.sourcePath, + filename: resolved.filename, + }); return { ok: true, path: dest }; } catch (err) { const reason = err instanceof Error ? err.message : String(err); @@ -145,6 +169,47 @@ export async function downloadArtifact( } } +/** + * Export an artifact via the native Save-As dialog (#3162), pre-filled + * with the artifact's filename. On the user dismissing the dialog, + * returns `{ ok: false, code: 'CANCELLED' }` (the caller should treat + * this as a no-op, not an error). If the dialog itself is unavailable — + * e.g. headless Linux with no xdg-desktop portal — falls back to the + * Downloads copy so the artifact is still recoverable. + */ +export async function saveArtifactViaDialog( + artifactId: string, + fallbackTitle: string, + extension: string +): Promise { + if (!isTauri()) { + return { ok: false, code: 'NOT_DESKTOP', error: 'Saving is only available in the desktop app' }; + } + + const resolved = await resolveArtifactForExport(artifactId, fallbackTitle, extension); + if (!resolved.ok) { + return { ok: false, code: resolved.code, error: resolved.error }; + } + + try { + // Command returns the saved path, or `null` when the user cancelled. + const dest = await invoke('save_artifact_via_dialog', { + sourcePath: resolved.sourcePath, + suggestedFilename: resolved.filename, + }); + if (dest == null) { + return { ok: false, code: 'CANCELLED', error: 'save cancelled by user' }; + } + return { ok: true, path: dest }; + } catch (err) { + // Dialog unavailable (no portal / unsupported) — recover the artifact + // via the Downloads copy rather than stranding the user. + const reason = err instanceof Error ? err.message : String(err); + console.warn('[artifact] save dialog failed, falling back to Downloads:', reason); + return downloadArtifact(artifactId, fallbackTitle, extension); + } +} + /** * Open the user's file manager pointed at the just-downloaded file. * Uses the existing `opener:allow-reveal-item-in-dir` capability — diff --git a/app/src/services/chatService.ts b/app/src/services/chatService.ts index ecf7a3baf..3f27fb01f 100644 --- a/app/src/services/chatService.ts +++ b/app/src/services/chatService.ts @@ -215,6 +215,26 @@ export interface ArtifactFailedEvent { error: string; } +/** + * Emitted when `artifacts::store::create_artifact` reserves an artifact + * row (status `Pending`), before the producer has written any bytes + * (#3162). The chat runtime upserts an `in_progress` snapshot keyed on + * `artifact_id` so the `ArtifactCard` shows a spinner the moment the + * tool starts, then swaps in place when the matching `artifact_ready` + * / `artifact_failed` arrives. Backend half shipped in #3277. + */ +export interface ArtifactPendingEvent { + thread_id: string; + client_id?: string; + artifact_id: string; + kind: ArtifactKind; + title: string; + /** Absolute workspace root — see {@link ArtifactReadyEvent.workspace_dir}. */ + workspace_dir: string; + /** Relative path under `/artifacts/`, e.g. `/deck.pptx`. */ + path: string; +} + /** Emitted when the agent turn begins (before the first LLM call). */ export interface ChatInferenceStartEvent { thread_id: string; @@ -458,6 +478,7 @@ export interface ChatEventListeners { onProactiveMessage?: (event: ProactiveMessageEvent) => void; onApprovalRequest?: (event: ChatApprovalRequestEvent) => void; onPlanReviewRequest?: (event: ChatPlanReviewRequestEvent) => void; + onArtifactPending?: (event: ArtifactPendingEvent) => void; onArtifactReady?: (event: ArtifactReadyEvent) => void; onArtifactFailed?: (event: ArtifactFailedEvent) => void; onDone?: (event: ChatDoneEvent) => void; @@ -494,6 +515,7 @@ export function subscribeChatEvents(listeners: ChatEventListeners): () => void { proactiveMessage: 'proactive_message', approvalRequest: 'approval_request', planReviewRequest: 'plan_review_request', + artifactPending: 'artifact_pending', artifactReady: 'artifact_ready', artifactFailed: 'artifact_failed', done: 'chat_done', @@ -861,6 +883,51 @@ export function subscribeChatEvents(listeners: ChatEventListeners): () => void { return { thread_id: env.thread_id, client_id, args }; }; + if (listeners.onArtifactPending) { + const cb = (payload: unknown) => { + const env = readEnvelope(payload); + if (!env) { + chatLog('%s — skipping malformed payload (bad envelope)', EVENTS.artifactPending); + return; + } + const { args } = env; + // Pending carries no size/path-on-disk yet — only identity + title. + if ( + !isNonEmptyString(args.artifact_id) || + !isValidArtifactKind(args.kind) || + !isNonEmptyString(args.title) || + !isNonEmptyString(args.workspace_dir) || + !isNonEmptyString(args.path) + ) { + chatLog( + '%s thread_id=%s — skipping malformed payload (bad args)', + EVENTS.artifactPending, + env.thread_id + ); + return; + } + const event: ArtifactPendingEvent = { + thread_id: env.thread_id, + client_id: env.client_id, + artifact_id: args.artifact_id, + kind: args.kind, + title: args.title, + workspace_dir: args.workspace_dir, + path: args.path, + }; + chatLog( + '%s thread_id=%s artifact_id=%s kind=%s', + EVENTS.artifactPending, + event.thread_id, + event.artifact_id, + event.kind + ); + listeners.onArtifactPending?.(event); + }; + socket.on(EVENTS.artifactPending, cb); + handlers.push([EVENTS.artifactPending, cb]); + } + if (listeners.onArtifactReady) { const cb = (payload: unknown) => { const env = readEnvelope(payload); @@ -1121,6 +1188,27 @@ export async function chatClearQueue(threadId: string): Promise { } } +/** + * Re-dispatch the producing tool for a failed artifact, reusing the same + * artifact id so the card swaps in place (#3162). Drives the failed-card + * Retry button: the core reloads the persisted creation args and re-runs + * the producer, routing the fresh pending/ready/failed events back to + * this thread + socket. Returns `true` when the RPC was accepted; the + * card's live state is then driven by the socket events, not this call. + */ +export async function aiRegenerate(artifactId: string, threadId: string): Promise { + const socket = socketService.getSocket(); + const clientId = socket?.id; + if (!clientId) { + throw new Error('Socket not connected — no client ID for event routing'); + } + await callCoreRpc({ + method: 'openhuman.ai_regenerate', + params: { artifact_id: artifactId, thread_id: threadId, client_id: clientId }, + }); + return true; +} + export function useRustChat(): boolean { // Legacy name kept for compatibility with existing call sites. return true; diff --git a/app/src/store/__tests__/chatRuntimeSlice.artifacts.test.ts b/app/src/store/__tests__/chatRuntimeSlice.artifacts.test.ts index 7fe836656..3a3b5dce5 100644 --- a/app/src/store/__tests__/chatRuntimeSlice.artifacts.test.ts +++ b/app/src/store/__tests__/chatRuntimeSlice.artifacts.test.ts @@ -234,3 +234,62 @@ describe('chatRuntimeSlice — artifact lifecycle (#2779)', () => { expect(list[0].error).toBeUndefined(); }); }); + +describe('chatRuntimeSlice — in_progress no-downgrade guard (#3162)', () => { + it('does NOT regress a ready artifact back to in_progress', () => { + let state = reducer( + undefined, + upsertArtifactReadyForThread({ + threadId: 't-1', + artifactId: 'a-1', + kind: 'presentation', + title: 'Deck', + path: 'a-1/deck.pptx', + sizeBytes: 4096, + }) + ); + + // A late / duplicate artifact_pending must not wipe the ready state. + state = reducer( + state, + upsertArtifactInProgressForThread({ + threadId: 't-1', + artifactId: 'a-1', + kind: 'presentation', + title: 'Deck', + }) + ); + + const list = state.artifactsByThread['t-1']; + expect(list).toHaveLength(1); + expect(list[0]).toMatchObject({ status: 'ready', sizeBytes: 4096 }); + }); + + it('allows failed -> in_progress (an explicit retry re-shows the spinner)', () => { + let state = reducer( + undefined, + upsertArtifactFailedForThread({ + threadId: 't-1', + artifactId: 'a-1', + kind: 'presentation', + title: 'Deck', + error: 'boom', + }) + ); + + state = reducer( + state, + upsertArtifactInProgressForThread({ + threadId: 't-1', + artifactId: 'a-1', + kind: 'presentation', + title: 'Deck', + }) + ); + + const list = state.artifactsByThread['t-1']; + expect(list).toHaveLength(1); + expect(list[0].status).toBe('in_progress'); + expect(list[0].error).toBeUndefined(); + }); +}); diff --git a/app/src/store/chatRuntimeSlice.ts b/app/src/store/chatRuntimeSlice.ts index bb7271b62..d22af566f 100644 --- a/app/src/store/chatRuntimeSlice.ts +++ b/app/src/store/chatRuntimeSlice.ts @@ -920,6 +920,20 @@ const chatRuntimeSlice = createSlice({ }> ) => { const { threadId, artifactId, kind, title } = action.payload; + // No-downgrade guard: a late `artifact_pending` (re-delivery, or a + // socket race) must never regress an artifact that already reached + // `ready` / `failed` back to a spinner. Only the regenerate flow + // (#3162) legitimately re-enters `in_progress`, and that reuses the + // id via a fresh pending event AFTER the failed state — which is + // allowed because the previous terminal state was `failed`, and a + // retry SHOULD show the spinner again. So: block downgrade only from + // `ready`; allow `failed -> in_progress` (an explicit retry). + const existing = (state.artifactsByThread[threadId] ?? []).find( + entry => entry.artifactId === artifactId + ); + if (existing && existing.status === 'ready') { + return; + } const snapshot: ArtifactSnapshot = { artifactId, kind, diff --git a/src/openhuman/artifacts/ops.rs b/src/openhuman/artifacts/ops.rs index 18618d7cd..892436437 100644 --- a/src/openhuman/artifacts/ops.rs +++ b/src/openhuman/artifacts/ops.rs @@ -1,9 +1,14 @@ use serde_json::{json, Value}; +use crate::openhuman::approval::{ApprovalChatContext, APPROVAL_CHAT_CONTEXT}; use crate::openhuman::config::Config; +use crate::openhuman::security::SecurityPolicy; +use crate::openhuman::tools::traits::Tool; +use crate::openhuman::tools::PresentationTool; use crate::rpc::RpcOutcome; use super::store; +use super::types::ArtifactKind; /// Default page size for `ai_list_artifacts`. const DEFAULT_LIMIT: usize = 50; @@ -123,6 +128,101 @@ pub async fn ai_delete_artifact( Ok(RpcOutcome::new(value, vec![])) } +/// Re-dispatch the producing tool for a failed (or any) artifact using +/// the args persisted at create-time, reusing the original `artifact_id` +/// so the in-chat card swaps in place (#3162). +/// +/// Drives the failed-card Retry affordance. The flow: +/// +/// 1. Load the artifact's `meta.json`; only `presentation` artifacts are +/// regenerable today (the single producing tool that persists args). +/// 2. Load the verbatim `args.json` sidecar — absent for artifacts made +/// before #3162 or by a non-persisting producer, which surfaces as a +/// "not regenerable" error rather than a silent no-op. +/// 3. Rebuild the producer tool + a fresh [`SecurityPolicy`] from the +/// live config and run it inside both: +/// - `REGENERATE_TARGET_ID.scope(artifact_id, …)` so `create_artifact` +/// reuses the id + directory instead of minting a new one, and +/// - `APPROVAL_CHAT_CONTEXT.scope({thread_id, client_id}, …)` so the +/// Pending/Ready/Failed events route back to the originating chat +/// surface (the RPC carries no ambient chat context of its own). +/// +/// The returned value is a thin ack — the card's live state is driven by +/// the socket events the re-run publishes, not by this RPC's result. +pub async fn ai_regenerate( + config: &Config, + artifact_id: &str, + thread_id: &str, + client_id: &str, +) -> Result, String> { + log::info!( + "[artifacts] ai_regenerate: id={artifact_id} thread_id={thread_id} client_id={client_id} workspace={:?}", + config.workspace_dir + ); + + if artifact_id.is_empty() { + return Err("[artifacts] artifact_id must not be empty".to_string()); + } + if thread_id.is_empty() || client_id.is_empty() { + return Err( + "[artifacts] regenerate requires thread_id + client_id for event routing".to_string(), + ); + } + + let meta = store::get_artifact(&config.workspace_dir, artifact_id).await?; + if meta.kind != ArtifactKind::Presentation { + return Err(format!( + "[artifacts] regenerate is only supported for presentations (artifact id={artifact_id} is {})", + meta.kind.as_str() + )); + } + + let args = store::read_artifact_args(&config.workspace_dir, artifact_id).await?; + + // A fresh policy from the live config — cheap, sync, and mirrors how + // the agent harness builds the same tool (see `tools::ops`). + let security = std::sync::Arc::new(SecurityPolicy::from_config( + &config.autonomy, + &config.workspace_dir, + &config.action_dir, + )); + let tool = PresentationTool::new(config.workspace_dir.clone(), security); + + let chat_ctx = ApprovalChatContext { + thread_id: thread_id.to_string(), + client_id: client_id.to_string(), + }; + + let result = store::REGENERATE_TARGET_ID + .scope( + artifact_id.to_string(), + APPROVAL_CHAT_CONTEXT.scope(chat_ctx, async move { tool.execute(args).await }), + ) + .await; + + match result { + Ok(tool_result) => { + // Even when the engine fails, `execute` returns `Ok(error)` and + // has already published `ArtifactFailed` for the reused id, so + // the card reflects the outcome via the socket. Report the flag + // back so the caller can log it. + log::info!( + "[artifacts] ai_regenerate: id={artifact_id} re-dispatched (is_error={})", + tool_result.is_error + ); + let value = json!({ + "artifact_id": artifact_id, + "regenerated": true, + "is_error": tool_result.is_error, + }); + Ok(RpcOutcome::new(value, vec![])) + } + Err(err) => Err(format!( + "[artifacts] regenerate execution error for id={artifact_id}: {err}" + )), + } +} + #[cfg(test)] #[path = "ops_tests.rs"] mod tests; diff --git a/src/openhuman/artifacts/ops_tests.rs b/src/openhuman/artifacts/ops_tests.rs index 99a3ca8e9..fd71e75a3 100644 --- a/src/openhuman/artifacts/ops_tests.rs +++ b/src/openhuman/artifacts/ops_tests.rs @@ -171,3 +171,105 @@ async fn delete_missing_id_error() { let err = ai_delete_artifact(&config, "").await.unwrap_err(); assert!(err.contains("must not be empty"), "unexpected error: {err}"); } + +// ── ai_regenerate (#3162) ────────────────────────────────────────────────── + +#[tokio::test] +async fn regenerate_rejects_empty_id() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + assert!(ai_regenerate(&config, "", "t", "c").await.is_err()); +} + +#[tokio::test] +async fn regenerate_requires_thread_and_client() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + assert!(ai_regenerate(&config, "id", "", "c").await.is_err()); + assert!(ai_regenerate(&config, "id", "t", "").await.is_err()); +} + +#[tokio::test] +async fn regenerate_rejects_non_presentation_kind() { + use crate::openhuman::artifacts::store::{save_artifact_args, save_artifact_meta}; + use crate::openhuman::artifacts::types::{ArtifactKind, ArtifactMeta, ArtifactStatus}; + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + save_artifact_meta( + tmp.path(), + &ArtifactMeta { + id: "doc-1".to_string(), + kind: ArtifactKind::Document, + title: "notes".to_string(), + path: "doc-1/notes.txt".to_string(), + size_bytes: 0, + status: ArtifactStatus::Failed, + created_at: chrono::Utc::now(), + error: Some("boom".to_string()), + thread_id: Some("t".to_string()), + }, + ) + .await + .unwrap(); + save_artifact_args(tmp.path(), "doc-1", &serde_json::json!({})) + .await + .unwrap(); + + let err = ai_regenerate(&config, "doc-1", "t", "c").await.unwrap_err(); + assert!( + err.contains("only supported for presentations"), + "unexpected error: {err}" + ); +} + +#[tokio::test] +async fn regenerate_errors_when_args_missing() { + use crate::openhuman::artifacts::store::create_artifact; + use crate::openhuman::artifacts::types::ArtifactKind; + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + // A presentation artifact with no persisted args.json (e.g. created + // before #3162) cannot be regenerated. + let (meta, _) = create_artifact(tmp.path(), ArtifactKind::Presentation, "Old Deck", "pptx") + .await + .unwrap(); + let err = ai_regenerate(&config, &meta.id, "t", "c") + .await + .unwrap_err(); + assert!(err.contains("not regenerable"), "unexpected error: {err}"); +} + +#[tokio::test] +async fn regenerate_reruns_producer_and_reuses_id() { + use crate::openhuman::artifacts::store::{create_artifact, get_artifact, save_artifact_args}; + use crate::openhuman::artifacts::types::{ArtifactKind, ArtifactStatus}; + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + // Seed a presentation artifact + its persisted creation args. + let (meta, _) = create_artifact(tmp.path(), ArtifactKind::Presentation, "Q3 Deck", "pptx") + .await + .unwrap(); + let args = serde_json::json!({ + "title": "Q3 Deck", + "slides": [{ "title": "Intro", "bullets": ["alpha", "beta"] }], + }); + save_artifact_args(tmp.path(), &meta.id, &args) + .await + .unwrap(); + + let outcome = ai_regenerate(&config, &meta.id, "thread-1", "client-1") + .await + .unwrap(); + let value = outcome.into_cli_compatible_json().unwrap(); + assert_eq!(value["artifact_id"], meta.id); + assert_eq!(value["regenerated"], true); + assert_eq!(value["is_error"], false); + + // Same id reused in place; the re-run drove it to Ready. + let got = get_artifact(tmp.path(), &meta.id).await.unwrap(); + assert_eq!(got.id, meta.id); + assert_eq!(got.status, ArtifactStatus::Ready); +} diff --git a/src/openhuman/artifacts/schemas.rs b/src/openhuman/artifacts/schemas.rs index 5a2c40a8e..59a3fa9dc 100644 --- a/src/openhuman/artifacts/schemas.rs +++ b/src/openhuman/artifacts/schemas.rs @@ -11,6 +11,7 @@ pub fn all_controller_schemas() -> Vec { schemas("list_artifacts"), schemas("get_artifact"), schemas("delete_artifact"), + schemas("regenerate"), ] } @@ -28,6 +29,10 @@ pub fn all_registered_controllers() -> Vec { schema: schemas("delete_artifact"), handler: handle_delete_artifact, }, + RegisteredController { + schema: schemas("regenerate"), + handler: handle_regenerate, + }, ] } @@ -171,6 +176,53 @@ pub fn schemas(function: &str) -> ControllerSchema { required: true, }], }, + "regenerate" => ControllerSchema { + namespace: "ai", + function: "regenerate", + description: "Re-run the producing tool for an existing artifact using its persisted creation args, reusing the same artifact id so the chat card swaps in place. Drives the failed-card Retry affordance (#3162).", + inputs: vec![ + artifact_id_input("Identifier of the artifact to regenerate."), + FieldSchema { + name: "thread_id", + ty: TypeSchema::String, + comment: "Chat thread to route the regenerated artifact's pending/ready/failed events to.", + required: true, + }, + FieldSchema { + name: "client_id", + ty: TypeSchema::String, + comment: "Socket client id (web channel) to address the regenerated artifact's events to.", + required: true, + }, + ], + outputs: vec![FieldSchema { + name: "result", + ty: TypeSchema::Object { + fields: vec![ + FieldSchema { + name: "artifact_id", + ty: TypeSchema::String, + comment: "Identifier that was regenerated (unchanged — reused in place).", + required: true, + }, + FieldSchema { + name: "regenerated", + ty: TypeSchema::Bool, + comment: "True when the producing tool was re-dispatched.", + required: true, + }, + FieldSchema { + name: "is_error", + ty: TypeSchema::Bool, + comment: "True when the re-dispatched generation itself reported an error (the card is flipped to failed via socket regardless).", + required: true, + }, + ], + }, + comment: "Regeneration ack payload.", + required: true, + }], + }, _other => ControllerSchema { namespace: "ai", function: "unknown", @@ -245,6 +297,24 @@ fn handle_delete_artifact(params: Map) -> ControllerFuture { }) } +fn handle_regenerate(params: Map) -> ControllerFuture { + Box::pin(async move { + let config = config_rpc::load_config_with_timeout().await?; + let artifact_id = read_required::(¶ms, "artifact_id")?; + let thread_id = read_required::(¶ms, "thread_id")?; + let client_id = read_required::(¶ms, "client_id")?; + to_json( + crate::openhuman::artifacts::ops::ai_regenerate( + &config, + artifact_id.trim(), + thread_id.trim(), + client_id.trim(), + ) + .await?, + ) + }) +} + fn read_required(params: &Map, key: &str) -> Result { let value = params .get(key) @@ -382,21 +452,48 @@ mod tests { .collect(); assert_eq!( names, - vec!["list_artifacts", "get_artifact", "delete_artifact"] + vec![ + "list_artifacts", + "get_artifact", + "delete_artifact", + "regenerate" + ] ); } #[test] fn all_registered_controllers_has_handler_per_schema() { let controllers = all_registered_controllers(); - assert_eq!(controllers.len(), 3); + assert_eq!(controllers.len(), 4); let names: Vec<_> = controllers.iter().map(|c| c.schema.function).collect(); assert_eq!( names, - vec!["list_artifacts", "get_artifact", "delete_artifact"] + vec![ + "list_artifacts", + "get_artifact", + "delete_artifact", + "regenerate" + ] ); } + #[test] + fn schemas_regenerate_requires_artifact_id_thread_and_client() { + let s = schemas("regenerate"); + assert_eq!(s.function, "regenerate"); + let input_names: Vec<_> = s.inputs.iter().map(|f| f.name).collect(); + assert_eq!(input_names, vec!["artifact_id", "thread_id", "client_id"]); + assert!(s.inputs.iter().all(|f| f.required)); + if let TypeSchema::Object { fields } = &s.outputs[0].ty { + let names: Vec<_> = fields.iter().map(|f| f.name).collect(); + assert!(names.contains(&"artifact_id")); + assert!(names.contains(&"regenerated")); + assert!(names.contains(&"is_error")); + } else { + panic!("expected object output type"); + } + } + // ── read_required ─────────────────────────────────────────────── #[test] diff --git a/src/openhuman/artifacts/store.rs b/src/openhuman/artifacts/store.rs index fb914f164..f895f9707 100644 --- a/src/openhuman/artifacts/store.rs +++ b/src/openhuman/artifacts/store.rs @@ -4,6 +4,22 @@ use super::types::{ArtifactMeta, ArtifactStatus}; const ARTIFACTS_SUBDIR: &str = "artifacts"; const META_FILENAME: &str = "meta.json"; +/// Sidecar file holding the verbatim producer-tool arguments that +/// generated the artifact, persisted next to `meta.json` so a failed +/// card's Retry button can re-dispatch the exact same generation +/// deterministically without round-tripping the args back through the +/// LLM (#3162). Written by the producing tool right after +/// [`create_artifact`]; read by `ops::ai_regenerate`. +const ARGS_FILENAME: &str = "args.json"; + +tokio::task_local! { + /// When set (by `ops::ai_regenerate`), [`create_artifact`] reuses + /// this id + its existing directory instead of minting a fresh + /// UUID — so a Retry swaps the failed card in place rather than + /// appending a second card (#3162). Unset for all normal + /// generation paths, in which case a fresh UUID is minted. + pub static REGENERATE_TARGET_ID: String; +} /// Returns the artifacts root directory, creating it if it doesn't exist. /// @@ -231,6 +247,67 @@ pub(crate) async fn get_artifact( Ok(meta) } +/// Persist the verbatim producer-tool arguments alongside an artifact's +/// `meta.json` as `/artifacts//args.json` (#3162). +/// +/// Stored so a later [`ops::ai_regenerate`](super::ops::ai_regenerate) +/// can reload the exact spec and re-run generation deterministically — +/// the Retry affordance on a failed card re-dispatches the *same* +/// request rather than asking the LLM to reconstruct it. Best-effort +/// from the producer's perspective: a write failure here does not fail +/// the generation, it only forfeits the ability to regenerate that +/// artifact. +pub(crate) async fn save_artifact_args( + workspace_dir: &Path, + artifact_id: &str, + args: &serde_json::Value, +) -> Result<(), String> { + log::debug!("[artifacts] save_artifact_args: id={artifact_id}"); + validate_artifact_id(artifact_id)?; + let root = artifacts_root(workspace_dir).await?; + let artifact_dir = root.join(artifact_id); + assert_within_root(&root, &artifact_dir)?; + tokio::fs::create_dir_all(&artifact_dir) + .await + .map_err(|e| { + format!( + "[artifacts] failed to create artifact dir {:?}: {e}", + artifact_dir + ) + })?; + let args_path = artifact_dir.join(ARGS_FILENAME); + let json = serde_json::to_string_pretty(args) + .map_err(|e| format!("[artifacts] failed to serialize args for id={artifact_id}: {e}"))?; + tokio::fs::write(&args_path, json) + .await + .map_err(|e| format!("[artifacts] failed to write args.json for id={artifact_id}: {e}"))?; + log::debug!("[artifacts] saved args.json for id={artifact_id}"); + Ok(()) +} + +/// Load the persisted producer-tool arguments for an artifact (#3162). +/// +/// Returns an `Err` when no `args.json` exists — the common case for +/// artifacts created before this sidecar was introduced, or by a +/// producer that never persisted args — so the caller can surface a +/// "cannot regenerate" message instead of silently doing nothing. +pub(crate) async fn read_artifact_args( + workspace_dir: &Path, + artifact_id: &str, +) -> Result { + log::debug!("[artifacts] read_artifact_args: id={artifact_id}"); + validate_artifact_id(artifact_id)?; + let root = artifacts_root(workspace_dir).await?; + let artifact_dir = root.join(artifact_id); + assert_within_root(&root, &artifact_dir)?; + let args_path = artifact_dir.join(ARGS_FILENAME); + let contents = tokio::fs::read_to_string(&args_path).await.map_err(|e| { + format!("[artifacts] no persisted args for id={artifact_id} (not regenerable): {e}") + })?; + serde_json::from_str(&contents) + .map_err(|e| format!("[artifacts] corrupt args.json for id={artifact_id}: {e}")) +} + /// Read the raw bytes of a finalized artifact's output file. /// /// Single source of truth for resolving an artifact id → on-disk bytes: @@ -369,7 +446,18 @@ pub async fn create_artifact( )); } - let id = uuid::Uuid::new_v4().to_string(); + // Normal path mints a fresh UUID. A regenerate (#3162) runs inside + // `REGENERATE_TARGET_ID.scope(...)` and reuses the original id so the + // Pending/Ready/Failed events that follow carry the same artifact_id + // and the card swaps in place. The reused dir already exists; the + // `create_dir_all` below is idempotent and the meta/file are + // overwritten with the fresh generation. + let reused_id = REGENERATE_TARGET_ID + .try_with(|target| target.clone()) + .ok() + .filter(|target| !target.trim().is_empty()); + let is_regenerate = reused_id.is_some(); + let id = reused_id.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); let filename = format!("{}.{trimmed_ext}", sanitize_filename_stem(trimmed_title)); let relative_path = format!("{id}/{filename}"); @@ -393,6 +481,19 @@ pub async fn create_artifact( // routing target survives a process restart. let (thread_id, _) = current_chat_context(); + // On a regenerate the id is reused in place, so preserve the original + // `created_at` — bumping it to now would reorder the artifact to the + // top of the `created_at`-sorted list/panel even though it is the same + // logical artifact (#3162, CodeRabbit). New artifacts always stamp now. + let created_at = if is_regenerate { + match get_artifact(workspace_dir, &id).await { + Ok(prev) => prev.created_at, + Err(_) => chrono::Utc::now(), + } + } else { + chrono::Utc::now() + }; + let meta = ArtifactMeta { id: id.clone(), kind, @@ -400,7 +501,7 @@ pub async fn create_artifact( path: relative_path, size_bytes: 0, status: ArtifactStatus::Pending, - created_at: chrono::Utc::now(), + created_at, error: None, thread_id, }; diff --git a/src/openhuman/artifacts/store_tests.rs b/src/openhuman/artifacts/store_tests.rs index c322b7fb2..e29268170 100644 --- a/src/openhuman/artifacts/store_tests.rs +++ b/src/openhuman/artifacts/store_tests.rs @@ -327,3 +327,84 @@ async fn create_artifact_publishes_artifact_pending_event() { assert!(thread_id.is_none(), "thread_id leaked, got {thread_id:?}"); assert!(client_id.is_none(), "client_id leaked, got {client_id:?}"); } + +// ── args sidecar + regenerate id reuse (#3162) ──────────────────────────── + +#[tokio::test] +async fn save_and_read_args_roundtrip() { + let tmp = TempDir::new().unwrap(); + let args = serde_json::json!({ + "title": "Q3 Deck", + "slides": [{ "heading": "Intro", "bullets": ["a", "b"] }], + }); + save_artifact_args(tmp.path(), "deck-1", &args) + .await + .unwrap(); + let got = read_artifact_args(tmp.path(), "deck-1").await.unwrap(); + assert_eq!(got, args); +} + +#[tokio::test] +async fn read_args_errors_when_absent() { + let tmp = TempDir::new().unwrap(); + // No args.json written → not regenerable, surfaces an Err rather than + // a silent empty value. + let err = read_artifact_args(tmp.path(), "missing").await.unwrap_err(); + assert!(err.contains("not regenerable"), "unexpected error: {err}"); +} + +#[tokio::test] +async fn create_artifact_mints_fresh_id_without_scope() { + let tmp = TempDir::new().unwrap(); + let (meta, _path) = create_artifact(tmp.path(), ArtifactKind::Presentation, "Q3 Deck", "pptx") + .await + .unwrap(); + // A normal (non-regenerate) create mints a UUID, never an empty id. + assert!(!meta.id.is_empty()); + assert_eq!(meta.status, ArtifactStatus::Pending); +} + +#[tokio::test] +async fn create_artifact_reuses_id_inside_regenerate_scope() { + let tmp = TempDir::new().unwrap(); + let workspace = tmp.path().to_path_buf(); + let (meta, _path) = REGENERATE_TARGET_ID + .scope("reused-id".to_string(), async move { + create_artifact(&workspace, ArtifactKind::Presentation, "Q3 Deck", "pptx").await + }) + .await + .unwrap(); + // The scoped target id is reused verbatim so the card swaps in place. + assert_eq!(meta.id, "reused-id"); + // And the meta is actually persisted under that id. + let got = get_artifact(tmp.path(), "reused-id").await.unwrap(); + assert_eq!(got.id, "reused-id"); +} + +#[tokio::test] +async fn regenerate_preserves_original_created_at() { + let tmp = TempDir::new().unwrap(); + let workspace = tmp.path().to_path_buf(); + + // First create stamps `created_at = now`. + let (first, _) = create_artifact(tmp.path(), ArtifactKind::Presentation, "Deck", "pptx") + .await + .unwrap(); + let original_created = first.created_at; + + // Regenerate reuses the id; created_at must NOT be bumped, otherwise the + // artifact jumps to the top of the created_at-sorted list (#3162). + let id = first.id.clone(); + let ws = workspace.clone(); + let (second, _) = REGENERATE_TARGET_ID + .scope(id.clone(), async move { + create_artifact(&ws, ArtifactKind::Presentation, "Deck", "pptx").await + }) + .await + .unwrap(); + assert_eq!(second.id, id); + assert_eq!( + second.created_at, original_created, + "regenerate must preserve created_at, not bump it" + ); +} diff --git a/src/openhuman/tools/impl/presentation/mod.rs b/src/openhuman/tools/impl/presentation/mod.rs index c6929e432..7bb253fce 100644 --- a/src/openhuman/tools/impl/presentation/mod.rs +++ b/src/openhuman/tools/impl/presentation/mod.rs @@ -244,6 +244,25 @@ impl Tool for PresentationTool { .await .map_err(anyhow::Error::msg)?; + // Persist the verbatim args next to meta.json so a failed card's + // Retry can re-dispatch this exact spec deterministically (#3162). + // Best-effort: a write failure only forfeits future regeneration, + // it must not abort an otherwise-successful generation. + if let Err(err) = crate::openhuman::artifacts::store::save_artifact_args( + &self.workspace_dir, + &meta.id, + &args, + ) + .await + { + tracing::warn!( + target: "presentation", + err = %err, + artifact_id = %meta.id, + "[presentation] failed to persist args.json; artifact will not be regenerable" + ); + } + let bytes = match engine::generate(&input, &resolved_images, GENERATION_TIMEOUT).await { Ok(bytes) => bytes, Err(err) => {