plugin: add top-5 missing tests + FFI edge cases (R2)

.beads/issues.jsonl

@@ -1,5 +1,5 @@
-{"_type":"issue","id":"dirge-woq","title":"R1: fix 3 critical plugin bugs (FFI panic, dialog deadlock, init hang)","description":"From the plugin subsystem audit: (1) wrap JanetCFunctions in catch_unwind so Rust panics don't unwind across the C-FFI boundary into Janet; (2) cancel send_dialog's reply_rx.recv() on worker shutdown so the worker thread doesn't block forever when the UI exits mid-dialog; (3) add timeout to the init handshake so a worker panic before init_tx.send() doesn't hang the main thread. Also: (4) bounds-assert wrap_string's i32 cast for the unlikely \u003e2GB case, (5) make take_string_slot atomic to close the race window, (6) don't eat unrelated user events in the dialog arm.","status":"in_progress","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-20T14:59:57Z","created_by":"Yogthos","updated_at":"2026-05-20T15:00:10Z","started_at":"2026-05-20T15:00:10Z","dependency_count":0,"dependent_count":1,"comment_count":0}
-{"_type":"issue","id":"dirge-f5m","title":"R2: add top-5 missing plugin tests + FFI edge cases","description":"From the plugin coverage audit: worker init-failure path, load_file with missing path, store_response round-trip, on-tool-end fires when inner returned Err, concurrent dispatch_tool_hook serialization. Plus FFI edge cases for read_string_arg (keyword/symbol/buffer), read_string_array_arg (empty + tuple-vs-array), wrap_string (empty / multibyte UTF-8). Depends on R1 so the worker init / dialog code is stable.","status":"open","priority":2,"issue_type":"task","owner":"yogthos@gmail.com","created_at":"2026-05-20T14:59:58Z","created_by":"Yogthos","updated_at":"2026-05-20T14:59:58Z","dependencies":[{"issue_id":"dirge-f5m","depends_on_id":"dirge-woq","type":"blocks","created_at":"2026-05-20T11:00:08Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0}
+{"_type":"issue","id":"dirge-woq","title":"R1: fix 3 critical plugin bugs (FFI panic, dialog deadlock, init hang)","description":"From the plugin subsystem audit: (1) wrap JanetCFunctions in catch_unwind so Rust panics don't unwind across the C-FFI boundary into Janet; (2) cancel send_dialog's reply_rx.recv() on worker shutdown so the worker thread doesn't block forever when the UI exits mid-dialog; (3) add timeout to the init handshake so a worker panic before init_tx.send() doesn't hang the main thread. Also: (4) bounds-assert wrap_string's i32 cast for the unlikely \u003e2GB case, (5) make take_string_slot atomic to close the race window, (6) don't eat unrelated user events in the dialog arm.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-20T14:59:57Z","created_by":"Yogthos","updated_at":"2026-05-20T15:30:28Z","started_at":"2026-05-20T15:00:10Z","closed_at":"2026-05-20T15:30:28Z","dependency_count":0,"dependent_count":1,"comment_count":0}
+{"_type":"issue","id":"dirge-f5m","title":"R2: add top-5 missing plugin tests + FFI edge cases","description":"From the plugin coverage audit: worker init-failure path, load_file with missing path, store_response round-trip, on-tool-end fires when inner returned Err, concurrent dispatch_tool_hook serialization. Plus FFI edge cases for read_string_arg (keyword/symbol/buffer), read_string_array_arg (empty + tuple-vs-array), wrap_string (empty / multibyte UTF-8). Depends on R1 so the worker init / dialog code is stable.","status":"in_progress","priority":2,"issue_type":"task","owner":"yogthos@gmail.com","created_at":"2026-05-20T14:59:58Z","created_by":"Yogthos","updated_at":"2026-05-20T15:30:28Z","started_at":"2026-05-20T15:30:28Z","dependencies":[{"issue_id":"dirge-f5m","depends_on_id":"dirge-woq","type":"blocks","created_at":"2026-05-20T11:00:08Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-6ip","title":"Phase 3b: Janet worker thread + harness/confirm + harness/select","description":"Move JanetClient onto a dedicated OS thread so harness/confirm and harness/select can block synchronously from Janet without deadlocking the current_thread tokio UI. Replaces the unsafe impl Send/Sync on PluginManager with a real worker channel. Adds DialogRequest channel the UI loop drains via tokio::select to render confirms and selects. Was originally part of Phase 3 but the refactor is large enough (~2 days) to warrant its own phase.","status":"closed","priority":2,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-20T14:04:20Z","created_by":"Yogthos","updated_at":"2026-05-20T14:43:32Z","started_at":"2026-05-20T14:22:58Z","closed_at":"2026-05-20T14:43:32Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-4rl","title":"Phase 4: plugin input transform via on-prompt return","description":"on-prompt hook returning a string can now replace (not just prepend) the user prompt. Backwards-compat: only replace when a new harness flag (harness/replace-prompt) is set. TDD: replace-prompt fires verbatim; default behavior preserved.","status":"closed","priority":2,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-20T13:27:29Z","created_by":"Yogthos","updated_at":"2026-05-20T14:07:28Z","started_at":"2026-05-20T14:04:49Z","closed_at":"2026-05-20T14:07:28Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-cp0","title":"Phase 3: UI primitives from Janet (notify/confirm/select)","description":"(harness/notify msg level), (harness/confirm title question), (harness/select title opts). Notify drains into a renderer queue. Confirm/select piggyback the existing AskRequest/QuestionResponse channels. Janet's !Send lock must be released around the oneshot await. TDD: notify queue ordering, confirm/select via mock channel.","status":"closed","priority":2,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-20T13:27:28Z","created_by":"Yogthos","updated_at":"2026-05-20T14:04:43Z","started_at":"2026-05-20T13:55:35Z","closed_at":"2026-05-20T14:04:43Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}

src/plugin/hook.rs

@@ -307,4 +307,73 @@ mod tests {
         let result = wrap_and_call(pm_arc, r#"{"x":2}"#).await.unwrap();
         assert_eq!(result, r#"{"x":2}"#);
     }
+
+    /// R2: on-tool-end fires even when the inner tool returned Err.
+    /// Plugins watching tool boundaries expect a symmetric start/end
+    /// pair regardless of outcome. The wrapper code is structured to
+    /// do this; this test pins it in place against accidental refactors
+    /// that might skip post-hook on error.
+    ///
+    /// We assert by having on-tool-end set a sentinel via
+    /// harness/replace-result — if the hook ran, the wrapper returns
+    /// Ok(sentinel) instead of the inner Err.
+    #[tokio::test]
+    async fn on_tool_end_fires_when_inner_returns_error() {
+        /// Tool that always fails. The wrapper should still call
+        /// `on-tool-end` after this, allowing the hook to substitute
+        /// a replacement output.
+        struct AlwaysFail;
+        impl ToolDyn for AlwaysFail {
+            fn name(&self) -> String {
+                "always_fail".to_string()
+            }
+            fn definition<'a>(
+                &'a self,
+                _prompt: String,
+            ) -> std::pin::Pin<Box<dyn std::future::Future<Output = ToolDefinition> + Send + 'a>>
+            {
+                Box::pin(async move {
+                    ToolDefinition {
+                        name: "always_fail".to_string(),
+                        description: "always errors".to_string(),
+                        parameters: serde_json::json!({}),
+                    }
+                })
+            }
+            fn call<'a>(
+                &'a self,
+                _args: String,
+            ) -> std::pin::Pin<
+                Box<dyn std::future::Future<Output = Result<String, ToolError>> + Send + 'a>,
+            > {
+                Box::pin(async move {
+                    Err(ToolError::ToolCallError(Box::<
+                        dyn std::error::Error + Send + Sync,
+                    >::from(
+                        "deliberate failure".to_string()
+                    )))
+                })
+            }
+        }
+
+        let pm_arc = pm();
+        {
+            let mut mgr = pm_arc.lock().unwrap();
+            // on-tool-end installs a replacement; if it doesn't fire
+            // when the inner tool errored, the wrapper would surface
+            // the underlying Err instead of this sentinel.
+            mgr.eval(
+                r#"(defn rewrite-error [ctx]
+                    (harness/replace-result "[error swallowed by plugin]"))"#,
+            )
+            .unwrap();
+            mgr.register("on-tool-end", "rewrite-error");
+        }
+
+        let wrapper = HookedToolDyn::with_manager(Box::new(AlwaysFail), Some(pm_arc));
+        let result = wrapper.call(String::new()).await;
+        // Plugin's replace-result rewrites the result regardless of
+        // inner success/failure, so we get Ok(replacement).
+        assert_eq!(result.unwrap(), "[error swallowed by plugin]");
+    }
 }

src/plugin/mod.rs

@@ -729,6 +729,100 @@ mod tests {
             .unwrap();
         assert_eq!(r, Some("he said \"hi\"\nline 2 \\ x".to_string()));
     }
+
+    // --- R2: coverage gaps from the audit ------------------------------
+
+    /// R2: load_file on a missing path surfaces an error rather than
+    /// panicking or silently succeeding.
+    #[cfg(feature = "plugin")]
+    #[test]
+    fn test_load_file_missing_path_returns_err() {
+        let mut mgr = PluginManager::try_new().unwrap();
+        let bogus = std::path::PathBuf::from("/tmp/dirge-nonexistent-plugin.janet");
+        // Make doubly sure it's not there.
+        let _ = std::fs::remove_file(&bogus);
+        let result = mgr.load_file(&bogus);
+        assert!(
+            result.is_err(),
+            "expected Err on missing file, got {result:?}"
+        );
+        let msg = result.unwrap_err();
+        assert!(
+            msg.contains("Failed to read plugin"),
+            "error should identify the read failure, got {msg:?}"
+        );
+    }
+
+    /// R2: store_response writes a slot that a subsequent eval can read.
+    /// Verifies the round-trip rather than just that the write doesn't
+    /// crash.
+    #[cfg(feature = "plugin")]
+    #[test]
+    fn test_store_response_round_trips_via_harness_var() {
+        let mut mgr = PluginManager::try_new().unwrap();
+        mgr.store_response("the assistant said this");
+        // harness-response is the slot store_response writes to.
+        let read = mgr.eval("harness-response").unwrap();
+        assert_eq!(read, "the assistant said this");
+    }
+
+    /// R2: store_response handles strings with Janet-special chars
+    /// (quotes, backslashes, newlines) without breaking the assignment.
+    #[cfg(feature = "plugin")]
+    #[test]
+    fn test_store_response_escapes_special_chars() {
+        let mut mgr = PluginManager::try_new().unwrap();
+        mgr.store_response("line one\n\"quoted\"\nline three \\ backslash");
+        let read = mgr.eval("harness-response").unwrap();
+        assert_eq!(read, "line one\n\"quoted\"\nline three \\ backslash");
+    }
+
+    /// R2: concurrent calls to `dispatch_tool_hook` via an
+    /// `Arc<Mutex<PluginManager>>` serialize cleanly. Two threads each
+    /// fire a unique-tagged hook; both should see their own block
+    /// reason come back in the result, with no interference. Catches
+    /// any future refactor that drops the lock mid-dispatch.
+    #[cfg(feature = "plugin")]
+    #[test]
+    fn test_concurrent_dispatch_tool_hook_serializes() {
+        use std::sync::{Arc, Mutex};
+
+        let pm = Arc::new(Mutex::new(PluginManager::try_new().unwrap()));
+        {
+            let mut mgr = pm.lock().unwrap();
+            mgr.eval(
+                r#"(defn block-by-tool [ctx]
+                    (harness/block (string "blocked:" (ctx :tool))))"#,
+            )
+            .unwrap();
+            mgr.register("on-tool-start", "block-by-tool");
+        }
+
+        // 8 concurrent threads each calling dispatch_tool_hook with a
+        // distinct :tool key. Without proper serialization a thread
+        // could observe another's slot value, mixing reasons.
+        let mut handles = Vec::new();
+        for i in 0..8 {
+            let pm = pm.clone();
+            handles.push(std::thread::spawn(move || {
+                let ctx = format!("@{{:tool \"t{i}\"}}");
+                let mut mgr = pm.lock().unwrap();
+                mgr.dispatch_tool_hook("on-tool-start", &ctx).unwrap()
+            }));
+        }
+
+        let mut reasons: Vec<String> = handles
+            .into_iter()
+            .filter_map(|h| h.join().ok())
+            .map(|r| r.block.unwrap_or_default())
+            .collect();
+        reasons.sort();
+        let expected: Vec<String> = (0..8).map(|i| format!("blocked:t{i}")).collect();
+        assert_eq!(
+            reasons, expected,
+            "each thread should see its own block reason"
+        );
+    }
 }
 
 use std::collections::HashMap;

src/plugin/worker.rs

@@ -786,4 +786,89 @@ mod tests {
         assert_eq!(r, "");
         helper.join().unwrap();
     }
+
+    // --- R2: FFI edge cases ---------------------------------------------
+
+    /// R2: read_string_arg accepts Janet keywords (call sites can use
+    /// `(harness/confirm :title "q")` instead of double-quoted strings).
+    /// Caught by an integration test through harness/confirm since the
+    /// cfn is the only caller; if read_string_arg ever stops accepting
+    /// keywords this test fails.
+    #[test]
+    fn confirm_accepts_keyword_title() {
+        let (mut worker, mut dialog_rx) = Worker::try_spawn().unwrap();
+        let helper = std::thread::spawn(move || match dialog_rx.blocking_recv() {
+            Some(DialogRequest::Confirm {
+                title,
+                question,
+                reply,
+            }) => {
+                assert_eq!(title, "warn");
+                assert_eq!(question, "really?");
+                let _ = reply.send(DialogReply::Confirm(true));
+            }
+            other => panic!("unexpected: {other:?}"),
+        });
+        // Keyword first arg — read_string_arg's is_kw branch handles it.
+        let r = worker
+            .eval(r#"(harness/__confirm :warn "really?")"#)
+            .unwrap();
+        assert_eq!(r, "true");
+        helper.join().unwrap();
+    }
+
+    /// R2: read_string_array_arg returns None for an empty array, and
+    /// the select cfn surfaces that as Janet nil. Janet-side
+    /// harness/select already short-circuits on `(indexed? opts)`, so
+    /// we hit the cfn via __select directly.
+    #[test]
+    fn select_with_empty_options_returns_nil() {
+        let (mut worker, _dialog_rx) = Worker::try_spawn().unwrap();
+        // Empty array should never even emit a dialog request.
+        let r = worker.eval(r#"(harness/__select "pick" [])"#).unwrap();
+        assert_eq!(r, "nil");
+    }
+
+    /// R2: read_string_array_arg works with tuples too (not just
+    /// arrays). Janet array literals `["a"]` are arrays; quoted forms
+    /// `'("a")` produce tuples. Both should be accepted.
+    #[test]
+    fn select_accepts_tuple_options() {
+        let (mut worker, mut dialog_rx) = Worker::try_spawn().unwrap();
+        let helper = std::thread::spawn(move || match dialog_rx.blocking_recv() {
+            Some(DialogRequest::Select { options, reply, .. }) => {
+                assert_eq!(options, vec!["alpha".to_string(), "beta".to_string()]);
+                let _ = reply.send(DialogReply::Select(Some("alpha".to_string())));
+            }
+            other => panic!("unexpected: {other:?}"),
+        });
+        // Use a quoted tuple instead of an array literal.
+        let r = worker
+            .eval(r#"(harness/__select "pick" '("alpha" "beta"))"#)
+            .unwrap();
+        assert!(r.contains("alpha"), "got {r:?}");
+        helper.join().unwrap();
+    }
+
+    /// R2: wrap_string handles multibyte UTF-8 correctly. The byte
+    /// length is the Janet string's allocation; an off-by-one here
+    /// would either truncate emoji or read past the slice.
+    #[test]
+    fn select_returns_multibyte_option_through_wrap_string() {
+        let (mut worker, mut dialog_rx) = Worker::try_spawn().unwrap();
+        let helper = std::thread::spawn(move || match dialog_rx.blocking_recv() {
+            Some(DialogRequest::Select { reply, .. }) => {
+                // Emoji + CJK + Cyrillic — all multibyte UTF-8.
+                let _ = reply.send(DialogReply::Select(Some("🦀漢字Привет".to_string())));
+            }
+            other => panic!("unexpected: {other:?}"),
+        });
+        let r = worker.eval(r#"(harness/select "pick" ["x"])"#).unwrap();
+        // Janet stringification preserves the raw UTF-8 bytes; the
+        // result should contain all three multibyte sequences intact.
+        assert!(r.contains("🦀"), "lost emoji: {r:?}");
+        assert!(r.contains("漢字"), "lost CJK: {r:?}");
+        assert!(r.contains("Привет"), "lost Cyrillic: {r:?}");
+        helper.join().unwrap();
+    }
 }