diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 5d8e7237..a9088029 100644 --- a/.beads/issues.jsonl +++ b/.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} diff --git a/src/plugin/hook.rs b/src/plugin/hook.rs index 33bc26a1..8d1599cc 100644 --- a/src/plugin/hook.rs +++ b/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 + 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> + 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]"); + } } diff --git a/src/plugin/mod.rs b/src/plugin/mod.rs index 7b39e298..fee0cf3d 100644 --- a/src/plugin/mod.rs +++ b/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>` 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 = handles + .into_iter() + .filter_map(|h| h.join().ok()) + .map(|r| r.block.unwrap_or_default()) + .collect(); + reasons.sort(); + let expected: Vec = (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; diff --git a/src/plugin/worker.rs b/src/plugin/worker.rs index 3fb0db99..30cfbe31 100644 --- a/src/plugin/worker.rs +++ b/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(); + } }