From caffc6fce0b5418492317a46b5bc4319fed04660 Mon Sep 17 00:00:00 2001 From: matt wilkie Date: Tue, 6 Oct 2026 23:01:21 -0700 Subject: [PATCH 1/3] Deepen watch mode into a debounce module C2 / Beads mdo-6w5.3 (epic mdo-6w5, PR order 3 of 6; stacked on the C6 branch). Watch mode's mechanics were interleaved with main.rs's CLI plumbing: parent-dir registration rationale, event relevance, the trailing-edge debounce drain, channel death, and the render-failure continuation all lived inline around ~180 lines with only two spawned-binary e2e tests as coverage, both unix-gated. New src/watch.rs in the lib crate owns all three mechanics behind one interface: - watch::run(input, render) - one deep watch entry point. Registers the watch BEFORE rendering (a save landing during the initial render is queued, not lost), runs the render callback once immediately, then once per settled burst. A render callback Err (transient filesystem state mid-rename) never ends watch mode; channel death ends it gracefully with Ok(()). - The debounce timing and the loop clock are injectable at pump level: unit tests script a fake event channel (relative arrival times) against a virtual clock, so the semantics are covered cross-platform by construction: burst collapse, bursts running longer than the window, events after the window, sibling noise, watcher error storms, channel death during a drain, callback errors, and event-relevance gates. - watch-mode UX lines (Watching / File changed, re-rendering / Watcher error) are the module's; rendering and its reporting stay with the CLI via the callback. main.rs keeps the CLI-local helpers (render_and_report, launch_open_- output) and its exit-code policy; the -watch -open launch hook stays a one-time duty consumed exactly on the first callback, as before the module existed. tests/cli.rs keeps one spawned-binary e2e smoke test (watch_survives_atomic_rename); the watch_ignores_sibling_changes e2e moves into the module unit tests. Gates: fmt --check, clippy --all-targets --all-features -D warnings, and cargo test --all-targets --all-features --locked all green (12 new watch unit tests; byte-identity harness against the pre-C1 binary still passes for the once-mode CLI surface). Beads-Ref: mdo-6w5.3 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt --- src/lib.rs | 4 +- src/main.rs | 304 ++++++++++------------------ src/watch.rs | 558 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/cli.rs | 58 +----- 4 files changed, 666 insertions(+), 258 deletions(-) create mode 100644 src/watch.rs diff --git a/src/lib.rs b/src/lib.rs index 26a262e..c962aed 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ -//! Shared mdo rendering, temp-output, browser launch, and integration helpers. +//! Shared mdo rendering, watch-mode, temp-output, browser launch, and +//! integration helpers. use std::collections::hash_map::DefaultHasher; use std::collections::BTreeSet; @@ -23,6 +24,7 @@ use url::Url; mod android; pub mod file_manager; mod highlight; +pub mod watch; #[cfg(target_os = "windows")] pub mod windows_setup; diff --git a/src/main.rs b/src/main.rs index bb60f10..c98e10a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,15 +33,13 @@ use std::io::{self, IsTerminal, Write}; use std::path::{Path, PathBuf}; -use std::sync::mpsc::channel; -use std::time::Duration; +use std::time::{Duration, Instant}; use clap::Parser; use mdo_cli::{ - convert, file_manager, launch_browser, open_setup_sample, temp_output_for, ConvertOutcome, - ConvertRequest, RenderError, StageTimings, + convert, file_manager, launch_browser, open_setup_sample, temp_output_for, watch, + ConvertOutcome, ConvertRequest, RenderError, StageTimings, }; -use notify::{recommended_watcher, EventKind, RecursiveMode, Watcher}; /// Markdown to HTML converter. Converts once by default; pass --watch to keep watching. #[derive(Parser)] @@ -398,190 +396,122 @@ fn main() -> notify::Result<()> { (None, false) => (input.with_extension("html"), false), }; - // Register the watch BEFORE the initial render so a save that lands - // while that render runs is queued by the watcher rather than silently - // lost; the watch loop below drains it and re-renders. Registration - // failure aborts before any rendering, which is fine — the user asked - // for a watch we cannot deliver. - // - // Watch the parent DIRECTORY rather than the file itself. Editors that - // save atomically (write a temp file, then rename it over the target) - // replace the target's inode; a watch on the file itself goes dead the - // moment that happens because the inode/handle notify was watching is - // gone. Watching the directory survives renames, deletes, and recreates - // — we just have to filter directory events down to ones that touch our - // target file. - // - // Resolve the target once, up front. Comparing later events against - // this fixed absolute path — rather than re-canonicalizing each event's - // path — matters because the target may momentarily not exist - // mid-rename, which would make canonicalize fail and misclassify a - // perfectly relevant event. - // - // Known limitation: canonicalizing a symlinked input means we watch the - // TARGET's parent, so edits made through the link are seen, but an - // editor atomically replacing the symlink itself is not. Watching both - // parents isn't worth the complexity until someone actually hits this. - let watch_setup = if args.watch { - let input_absolute = std::fs::canonicalize(&input).unwrap_or_else(|_| { - std::env::current_dir() - .map(|cwd| cwd.join(&input)) - .unwrap_or_else(|_| input.clone()) + if args.watch { + // Watch's contract: register before rendering (a save that lands + // while the initial render runs is queued by the watcher rather + // than silently lost), run the render callback once immediately, + // then pump. In watch mode we deliberately do NOT apply the one-shot + // exit-code check below: a one-time --open failure must not end + // watch mode, since the whole point is to keep re-rendering on + // future edits. + let mut pending_open_launch = args.open; + return watch::run(&input, &mut || { + let result = render_and_report( + &input, + &output, + args.bare, + args.unsafe_html, + args.css.as_deref(), + private_output, + args.verbose, + ); + if pending_open_launch { + // The launch hook is a one-time post-initial-render duty + // (the pump runs the callback immediately): consume the + // pending flag on the first callback whether or not that + // render succeeded, exactly as the pre-module flow did. + if result.is_ok() { + launch_open_output(&output, args.verbose); + } + pending_open_launch = false; + } + result.map(|_| ()) }); - let watch_dir = input_absolute - .parent() - .map(PathBuf::from) - .unwrap_or_else(|| PathBuf::from(".")); - let target_file_name = input_absolute.file_name().map(|n| n.to_os_string()); - - let (tx, rx) = channel(); - let mut watcher = recommended_watcher(tx)?; - watcher.watch(&watch_dir, RecursiveMode::NonRecursive)?; - Some((watcher, rx, watch_dir, target_file_name)) - } else { - None - }; + } - let converted = report_render( + let converted = render_and_report( &input, &output, - &convert(ConvertRequest { - input: &input, - output: &output, - bare: args.bare, - unsafe_html: args.unsafe_html, - private_output, - css_override: args.css.as_deref(), - }), + args.bare, + args.unsafe_html, + args.css.as_deref(), + private_output, args.verbose, - ); + ) + .is_ok(); // Track whether --open promised a browser launch and failed to deliver // one, so the one-shot exit code below can be honest about it. let mut open_launch_failed = false; if args.open && converted { - // Browser launch is reported separately from the render workflow: - // launch_browser only initiates the launch (fire-and-forget), so - // this measures initiation, not the browser starting up. - let launch_start = std::time::Instant::now(); - match launch_browser(&output) { - Ok(()) => { - if args.verbose { - eprintln!( - "⏱ Browser launch initiated in {:.3} ms", - launch_start.elapsed().as_secs_f64() * 1000.0 - ); - } - println!("🌐 Opened {:?} in default browser", output); - } - Err(e) => { - // The render already succeeded and nothing removes the - // rendered file on a launch failure, so point the user at it - // instead of silently downgrading this to a warning. - eprintln!("❌ Failed to launch browser: {}", e); - eprintln!(" Rendered output is available at {:?}", output); - open_launch_failed = true; - } - } + open_launch_failed = launch_open_output(&output, args.verbose); } - if !args.watch { - // Exit non-zero on a failed one-shot render so scripts and the docs - // pipeline can detect errors. A failed --open browser launch is the - // same kind of broken promise even though the render itself - // succeeded: `mdo --open` only fully succeeds when the file is both - // rendered AND opened, so it exits non-zero here too. In watch mode - // we deliberately do NOT apply this exit-code check: watch's - // contract is to keep running regardless of a one-time --open - // failure, since the whole point is to keep re-rendering on future - // edits. The stderr message above still fires so the user learns - // about the failed launch either way. - if converted && !open_launch_failed { - return Ok(()); - } - std::process::exit(1); + // Exit non-zero on a failed one-shot render so scripts and the docs + // pipeline can detect errors. A failed --open browser launch is the + // same kind of broken promise even though the render itself succeeded: + // `mdo --open` only fully succeeds when the file is both rendered AND + // opened, so it exits non-zero here too. + if converted && !open_launch_failed { + return Ok(()); } + std::process::exit(1); +} - let (_watcher, rx, watch_dir, target_file_name) = - watch_setup.expect("watch setup was built above whenever --watch is set"); - - // Still named after the file, not the directory: that's what the user - // asked to watch, even though the underlying notify watch is scoped one - // level up. - println!("👀 Watching {:?} for changes... (Ctrl+C to stop)", input); - - // Trailing-edge debounce window: once a relevant event arrives we keep - // draining/absorbing further relevant events for this long before - // rendering, so a burst (e.g. truncate + write, or temp-file write + - // rename) collapses into exactly one render of the final content. This - // is unlike a leading-edge "ignore anything for N ms after the last - // render" debounce, which can drop the trailing event of a burst if the - // burst runs longer than the window. - const DEBOUNCE: Duration = Duration::from_millis(200); - - loop { - // Block for the next event. Anything that isn't a relevant, - // content-changing event (wrong file, Access events, watcher - // errors) is skipped without starting a debounce window. - let event = match rx.recv() { - Ok(Ok(event)) => event, - Ok(Err(e)) => { - eprintln!("⚠️ Watcher error: {}", e); - continue; - } - Err(_) => return Ok(()), // watcher/channel gone; nothing left to watch - }; - - if !is_relevant_event(&event, target_file_name.as_deref(), &watch_dir) { - continue; - } +/// Run one render request and report it. All rendering output lives behind +/// `report_render`, so the one-shot path and watch-mode re-renders report +/// identically. The typed outcome still comes back: the watch callback +/// feeds its `Err` into `watch::run`, which must not end watch mode on a +/// transient filesystem state mid-rename. +fn render_and_report( + input: &Path, + output: &Path, + bare: bool, + unsafe_html: bool, + css_override: Option<&Path>, + private_output: bool, + verbose: bool, +) -> Result<(), RenderError> { + let outcome = convert(ConvertRequest { + input, + output, + bare, + unsafe_html, + private_output, + css_override, + }); + report_render(input, output, &outcome, verbose); + outcome.map(|_| ()) +} - // Drain/absorb further relevant events for a quiet window, then - // render once. This covers direct writes, truncate+rewrite, and - // atomic saves (temp file write followed by a rename onto the - // target) uniformly: whichever event kinds the editor and platform - // happen to emit (Create, Modify, Rename-as-Modify(Name), Remove - // followed by a Create), each just extends the quiet window until - // the burst settles. Only RELEVANT events extend the deadline: - // sibling-file noise merely waits out the remaining window, so a - // busy directory (a build churning artifacts next to the watched - // file) cannot postpone the render indefinitely. - let mut deadline = std::time::Instant::now() + DEBOUNCE; - loop { - let remaining = deadline.saturating_duration_since(std::time::Instant::now()); - if remaining.is_zero() { - break; - } - match rx.recv_timeout(remaining) { - Ok(Ok(event)) => { - if is_relevant_event(&event, target_file_name.as_deref(), &watch_dir) { - deadline = std::time::Instant::now() + DEBOUNCE; - } - // Irrelevant events neither extend nor cut the window. - } - Ok(Err(e)) => eprintln!("⚠️ Watcher error: {}", e), - Err(_) => break, // quiet window elapsed (or channel closed) +/// `--open`'s post-render launch hook: initiate the browser launch and +/// report it. Returns whether the launch failed to deliver on the promise — +/// the one-shot exit code owes this honesty; watch mode ignores failures +/// and keeps watching to re-render future edits. +fn launch_open_output(output: &Path, verbose: bool) -> bool { + // Browser launch is reported separately from the render workflow: + // launch_browser only initiates the launch (fire-and-forget), so + // this measures initiation, not the browser starting up. + let launch_start = Instant::now(); + match launch_browser(output) { + Ok(()) => { + if verbose { + eprintln!( + "⏱ Browser launch initiated in {:.3} ms", + launch_start.elapsed().as_secs_f64() * 1000.0 + ); } + println!("🌐 Opened {:?} in default browser", output); + false + } + Err(e) => { + // The render already succeeded and nothing removes the + // rendered file on a launch failure, so point the user at it + // instead of silently downgrading this to a warning. + eprintln!("❌ Failed to launch browser: {}", e); + eprintln!(" Rendered output is available at {:?}", output); + true } - - println!("🔁 File changed, re-rendering..."); - // A failed render (e.g. convert returned Err while the file was - // momentarily absent mid-rename) must not end watch mode; the ❌ - // error is reported by `report_render`, so we just loop and wait - // for the next event. - let _ = report_render( - &input, - &output, - &convert(ConvertRequest { - input: &input, - output: &output, - bare: args.bare, - unsafe_html: args.unsafe_html, - private_output, - css_override: args.css.as_deref(), - }), - args.verbose, - ); } } @@ -643,36 +573,6 @@ fn report_render_timings(input: &Path, timings: &StageTimings) { eprintln!(" total {:>10.3} ms", ms(timings.total)); } -/// True if `event` plausibly changed the content of the file named -/// `target_file_name` inside `watch_dir`. We match by file name (and, when -/// notify reports one, by parent directory) rather than canonicalizing the -/// event's path, since the target can momentarily not exist mid-rename. -/// Access events (reads, permission-bit-only changes) are excluded; every -/// other kind (Create, Modify, Remove, Any, Other) is treated as a possible -/// content change so recreation-after-remove and atomic renames are all -/// covered by the same check. -fn is_relevant_event( - event: ¬ify::Event, - target_file_name: Option<&std::ffi::OsStr>, - watch_dir: &std::path::Path, -) -> bool { - if matches!(event.kind, EventKind::Access(_)) { - return false; - } - - let Some(target_file_name) = target_file_name else { - return false; - }; - - event.paths.iter().any(|p| { - p.file_name() == Some(target_file_name) - && match p.parent() { - Some(parent) => parent == watch_dir, - None => true, - } - }) -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/watch.rs b/src/watch.rs new file mode 100644 index 0000000..6ed723a --- /dev/null +++ b/src/watch.rs @@ -0,0 +1,558 @@ +//! Watch-mode mechanics, behind one interface: folder watching, event +//! relevance, and the trailing-edge debounce. +//! +//! The module owns exactly the three mechanics `mdo --watch` needs; the +//! render itself and how the CLI reports it belong to the caller, passed as +//! a render callback. `watch::run` registers the watch, runs the callback +//! once immediately, and then once per settled quiet window whenever the +//! watched file plausibly changed content. A callback `Err` (e.g. a +//! transient filesystem state mid-rename) is ignored and never ends watch +//! mode, matching the CLI's contract; channel death ends the watch +//! gracefully. +//! +//! Both the debounce timing and the loop clock are injectable at the pump +//! level, so unit tests script fake event channels with a virtual clock and +//! cover burst collapse, late events, sibling noise, watcher error storms, +//! and channel death cross-platform by construction. + +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; +use std::sync::mpsc::{self, channel, RecvError, RecvTimeoutError}; +use std::time::{Duration, Instant}; + +use notify::{recommended_watcher, Event, EventKind, RecursiveMode, Watcher}; + +use crate::RenderError; + +/// Trailing-edge debounce window: once a relevant event arrives the pump +/// keeps draining/absorbing further relevant events for this long before +/// rendering, so a burst (e.g. truncate + write, or temp-file write + +/// rename) collapses into exactly one render of the final content. This is +/// unlike a leading-edge "ignore anything for N ms after the last render" +/// debounce, which can drop the trailing event of a burst if the burst runs +/// longer than the window. +const DEFAULT_DEBOUNCE: Duration = Duration::from_millis(200); + +/// Run watch mode for `input`: register a parent-directory watch, run +/// `render` once immediately, then re-run it once per settled burst of +/// events that plausibly changed the file's content. +/// +/// `render` runs once immediately — before any event is seen — so callers +/// that must do a one-time thing after the initial render (e.g. the +/// `--open` browser launch) guard that in their own callback state. Its +/// `Err` is ignored: a failed render must never end watch mode. +/// +/// Errors propagate only from registration itself, before any render: a +/// watch we cannot deliver aborts up front rather than rendering once and +/// failing later. Once pumping, the loop returns `Ok(())` only when the +/// watcher channel dies; it otherwise runs until killed. +pub fn run Result<(), RenderError>>( + input: &Path, + render: &mut F, +) -> notify::Result<()> { + // Watch the parent DIRECTORY rather than the file itself. Editors that + // save atomically (write a temp file, then rename it over the target) + // replace the target's inode; a watch on the file itself goes dead the + // moment that happens because the inode/handle notify was watching is + // gone. Watching the directory survives renames, deletes, and recreates + // — we just have to filter directory events down to ones that touch our + // target file. + // + // Resolve the target once, up front. Comparing later events against + // this fixed absolute path — rather than re-canonicalizing each event's + // path — matters because the target may momentarily not exist + // mid-rename, which would make canonicalize fail and misclassify a + // perfectly relevant event. + // + // Known limitation: canonicalizing a symlinked input means we watch the + // TARGET's parent, so edits made through the link are seen, but an + // editor atomically replacing the symlink itself is not. Watching both + // parents isn't worth the complexity until someone actually hits this. + let input_absolute = std::fs::canonicalize(input).unwrap_or_else(|_| { + std::env::current_dir() + .map(|cwd| cwd.join(input)) + .unwrap_or_else(|_| input.to_path_buf()) + }); + let watch_dir = input_absolute + .parent() + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from(".")); + let target_file_name = input_absolute.file_name().map(|n| n.to_os_string()); + + // Register the watch BEFORE the initial render so a save that lands + // while that render runs is queued by the watcher rather than silently + // lost; the pump below drains it and re-renders. Registration + // failure aborts before any rendering, which is fine — the user asked + // for a watch we cannot deliver. + + let (tx, rx) = channel(); + let mut watcher = recommended_watcher(tx)?; + watcher.watch(&watch_dir, RecursiveMode::NonRecursive)?; + + let _ = render(); + + // Still named after the file, not the directory: that's what the user + // asked to watch, even though the underlying notify watch is scoped one + // level up. + println!("👀 Watching {:?} for changes... (Ctrl+C to stop)", input); + + pump( + &rx, + &watch_dir, + target_file_name.as_deref(), + render, + DEFAULT_DEBOUNCE, + &SystemClock, + ); + Ok(()) +} + +/// The event pump: block for the next event, skip everything that isn't a +/// relevant change to the target file, drain/absorb bursts through the +/// trailing-edge debounce window, then render once per settled burst. +/// +/// `debounce` and `clock` are the injectable timing seams (unit tests +/// script relative arrival times against a virtual clock; production uses +/// the system monotonic clock). +fn pump Result<(), RenderError>, C: Clock>( + rx: &R, + watch_dir: &Path, + target_file_name: Option<&OsStr>, + render: &mut F, + debounce: Duration, + clock: &C, +) { + loop { + // Block for the next event. Anything that isn't a relevant, + // content-changing event (wrong file, Access events, watcher + // errors) is skipped without starting a debounce window. + let event = match rx.recv() { + Ok(Ok(event)) => event, + Ok(Err(e)) => { + eprintln!("⚠️ Watcher error: {}", e); + continue; + } + Err(_) => return, // watcher/channel gone; nothing left to watch + }; + + if !is_relevant_event(&event, target_file_name, watch_dir) { + continue; + } + + // Drain/absorb further relevant events for a quiet window, then + // render once. This covers direct writes, truncate+rewrite, and + // atomic saves (temp file write followed by a rename onto the + // target) uniformly: whichever event kinds the editor and platform + // happen to emit (Create, Modify, Rename-as-Modify(Name), Remove + // followed by a Create), each just extends the quiet window until + // the burst settles. Only RELEVANT events extend the deadline: + // sibling-file noise merely waits out the remaining window, so a + // busy directory (a build churning artifacts next to the watched + // file) cannot postpone the render indefinitely. + let mut deadline = clock.now() + debounce; + loop { + let remaining = deadline.saturating_duration_since(clock.now()); + if remaining.is_zero() { + break; + } + match rx.recv_timeout(remaining) { + Ok(Ok(event)) => { + if is_relevant_event(&event, target_file_name, watch_dir) { + deadline = clock.now() + debounce; + } + // Irrelevant events neither extend nor cut the window. + } + Ok(Err(e)) => eprintln!("⚠️ Watcher error: {}", e), + Err(_) => break, // quiet window elapsed (or channel closed) + } + } + + println!("🔁 File changed, re-rendering..."); + // A failed render (e.g. one that ran while the file was momentarily + // absent mid-rename) must not end watch mode; its reporting already + // happened inside the callback, so just loop and wait for the next + // event. + let _ = render(); + } +} + +/// True if `event` plausibly changed the content of the file named +/// `target_file_name` inside `watch_dir`. We match by file name (and, when +/// notify reports one, by parent directory) rather than canonicalizing the +/// event's path, since the target can momentarily not exist mid-rename. +/// Access events (reads, permission-bit-only changes) are excluded; every +/// other kind (Create, Modify, Remove, Any, Other) is treated as a possible +/// content change so recreation-after-remove and atomic renames are all +/// covered by the same check. +fn is_relevant_event(event: &Event, target_file_name: Option<&OsStr>, watch_dir: &Path) -> bool { + if matches!(event.kind, EventKind::Access(_)) { + return false; + } + + let Some(target_file_name) = target_file_name else { + return false; + }; + + event.paths.iter().any(|p| { + p.file_name() == Some(target_file_name) + && match p.parent() { + Some(parent) => parent == watch_dir, + None => true, + } + }) +} + +/// One item on the watch event channel: a real event, or a watcher-level +/// error delivered as a channel payload (the queue keeps flowing either +/// way). +type EventItem = Result; + +/// The receiving half of the watch event channel narrowed to the exact +/// shape the pump uses. Abstracted so unit tests script fake sequences. +trait EventRx { + fn recv(&self) -> Result; + fn recv_timeout(&self, timeout: Duration) -> Result; +} + +impl EventRx for mpsc::Receiver { + fn recv(&self) -> Result { + mpsc::Receiver::recv(self) + } + + fn recv_timeout(&self, timeout: Duration) -> Result { + mpsc::Receiver::recv_timeout(self, timeout) + } +} + +/// Where the pump reads its clock. Production uses the system monotonic +/// clock; tests inject a virtual one. +trait Clock { + fn now(&self) -> Instant; +} + +struct SystemClock; + +impl Clock for SystemClock { + fn now(&self) -> Instant { + Instant::now() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::cell::Cell; + use std::cell::RefCell; + use std::collections::VecDeque; + use std::rc::Rc; + + use notify::event::{AccessKind, ModifyKind}; + + /// Virtual monotonic clock shared with the fake channel: entries in a + /// script declare how long after "now" their payload is ready. + #[derive(Clone)] + struct VirtualClock(Rc>); + + impl VirtualClock { + fn new() -> Self { + Self(Rc::new(Cell::new(Instant::now()))) + } + + fn advance(&self, by: Duration) { + self.0.set(self.0.get() + by); + } + } + + impl Clock for VirtualClock { + fn now(&self) -> Instant { + self.0.get() + } + } + + /// One scripted channel entry: a payload plus how long after the + /// current virtual time it becomes available. + struct Scripted { + ready_after: Duration, + payload: EventItem, + } + + /// Fake channel half driving `pump` from a script. Events ready within + /// the polled timeout are delivered on that poll; anything later goes + /// past the window (timed out), exactly as a real channel would. + struct FakeRx { + clock: VirtualClock, + script: RefCell>, + } + + impl FakeRx { + fn now_or_elapsed(&self, timeout: Duration) -> Result { + let next = self.script.borrow().front().map(|s| s.ready_after); + match next { + Some(ready_after) if ready_after <= timeout => { + self.clock.advance(ready_after); + let scripted = self + .script + .borrow_mut() + .pop_front() + .expect("front checked above"); + Ok(scripted.payload) + } + _ => { + self.clock.advance(timeout); + Err(RecvTimeoutError::Timeout) + } + } + } + } + + impl EventRx for FakeRx { + fn recv(&self) -> Result { + match self.script.borrow_mut().pop_front() { + Some(scripted) => { + self.clock.advance(scripted.ready_after); + Ok(scripted.payload) + } + None => Err(RecvError), + } + } + + fn recv_timeout(&self, timeout: Duration) -> Result { + self.now_or_elapsed(timeout) + } + } + + fn event_ready_after(ready_after: u64, kind: EventKind, path: PathBuf) -> Scripted { + Scripted { + ready_after: Duration::from_millis(ready_after), + payload: Ok(Event::new(kind).add_path(path)), + } + } + + fn watcher_error_after(ready_after: u64) -> Scripted { + Scripted { + ready_after: Duration::from_millis(ready_after), + payload: Err(notify::Error::io(std::io::Error::other("watcher failed"))), + } + } + + /// A content-changing event for `name` inside `watch_dir`. + fn modify(name: &str, watch_dir: &Path, ready_after: u64) -> Scripted { + event_ready_after( + ready_after, + EventKind::Modify(ModifyKind::Any), + watch_dir.join(name), + ) + } + + /// A change to a file that is not the watched target. + fn sibling(name: &str, watch_dir: &Path, ready_after: u64) -> Scripted { + event_ready_after( + ready_after, + EventKind::Modify(ModifyKind::Any), + watch_dir.join(name), + ) + } + + fn access(watch_dir: &Path, ready_after: u64) -> Scripted { + event_ready_after( + ready_after, + EventKind::Access(AccessKind::Any), + watch_dir.join("sample.md"), + ) + } + + /// Pump over a script, counting renders in the callback. + fn pump_scripted(script: Vec) -> (usize, VirtualClock) { + let clock = VirtualClock::new(); + let renders = Rc::new(Cell::new(0_usize)); + let rx = FakeRx { + clock: clock.clone(), + script: RefCell::new(script.into()), + }; + pump( + &rx, + Path::new("/watch"), + Some(OsStr::new("sample.md")), + &mut || { + renders.set(renders.get() + 1); + Ok(()) + }, + Duration::from_millis(200), + &clock, + ); + (renders.get(), clock) + } + + #[test] + fn burst_collapses_into_one_render() { + // Three events inside one window: one render of the final content. + let (renders, _clock) = pump_scripted(vec![ + modify("sample.md", Path::new("/watch"), 50), + modify("sample.md", Path::new("/watch"), 70), + modify("sample.md", Path::new("/watch"), 60), + ]); + assert_eq!(renders, 1, "a settled burst renders exactly once"); + } + + #[test] + fn burst_running_longer_than_window_still_renders_the_trailing_event() { + // Events every 80ms outrun a 200ms quiet window for a while; no + // render fires mid-burst — the trailing event is never dropped. + let (renders, _clock) = pump_scripted(vec![ + modify("sample.md", Path::new("/watch"), 50), + modify("sample.md", Path::new("/watch"), 80), + modify("sample.md", Path::new("/watch"), 80), + modify("sample.md", Path::new("/watch"), 80), + modify("sample.md", Path::new("/watch"), 80), + ]); + assert_eq!(renders, 1, "a long burst settles into exactly one render"); + } + + #[test] + fn event_after_the_window_renders_again() { + // One burst, then quiet past the window, then another event: the + // first event settles into render one; the late event is NOT + // dropped — it becomes the next outer event and renders again. + let (renders, _clock) = pump_scripted(vec![ + modify("sample.md", Path::new("/watch"), 50), + modify("sample.md", Path::new("/watch"), 700), + ]); + assert_eq!(renders, 2, "events after the window are fresh triggers"); + } + + #[test] + fn sibling_noise_renders_nothing() { + // Sibling-file churn alone never triggers or extends a window. + let (renders, _clock) = pump_scripted(vec![ + sibling("other.md", Path::new("/watch"), 50), + sibling("build.log", Path::new("/watch"), 80), + ]); + assert_eq!(renders, 0, "no target file in any event, no render"); + } + + #[test] + fn sibling_noise_waits_out_but_does_not_extend_the_window() { + // A sibling event arriving well past the current window cannot + // extend the deadline: the pump waits out only the remaining window + // (bounded by one debounce) and renders; the sibling is then seen + // as the next outer event, classified irrelevant, and skipped. + let (renders, _clock) = pump_scripted(vec![ + modify("sample.md", Path::new("/watch"), 50), + sibling("other.md", Path::new("/watch"), 1000), + ]); + assert_eq!(renders, 1, "the sibling burst never postponed the render"); + } + + #[test] + fn access_events_are_never_relevant() { + let (renders, _clock) = pump_scripted(vec![access(Path::new("/watch"), 50)]); + assert_eq!(renders, 0, "Access events are excluded"); + } + + #[test] + fn channel_death_before_any_event_ends_the_watch_gracefully() { + let (renders, _clock) = pump_scripted(vec![]); + assert_eq!(renders, 0); + } + + #[test] + fn channel_death_during_drain_renders_and_exits() { + // An event lands, the window is open, the channel dies: the + // pending burst still renders, then the watch ends. + let (renders, _clock) = pump_scripted(vec![modify("sample.md", Path::new("/watch"), 50)]); + assert_eq!(renders, 1); + } + + #[test] + fn watcher_error_storm_renders_nothing_and_survives() { + let script: Vec = (0..50u64) + .map(|i| watcher_error_after(i * 10 + 1)) + .collect(); + let (renders, _clock) = pump_scripted(script); + assert_eq!(renders, 0, "watcher errors are not renders"); + } + + #[test] + fn watcher_errors_inside_the_drain_do_not_extend_the_window() { + // Errors arriving inside a window neither extend nor cut it: they + // wait out the remaining window like any other noise. + let script = vec![ + modify("sample.md", Path::new("/watch"), 50), + watcher_error_after(60), + watcher_error_after(60), + ]; + let (renders, _clock) = pump_scripted(script); + assert_eq!(renders, 1); + } + + #[test] + fn callback_errors_never_end_watch_mode() { + // A render callback that reports a transient filesystem failure + // (e.g. the file was absent mid-rename) must not end watch mode. + let clock = VirtualClock::new(); + let renders = Rc::new(Cell::new(0_usize)); + let failures = Rc::new(Cell::new(0_usize)); + let rx = FakeRx { + clock: clock.clone(), + script: RefCell::new( + vec![ + // Two bursts separated past the window: render One and + // render two must BOTH happen for the errors never to + // have ended watch mode. + modify("sample.md", Path::new("/watch"), 50), + modify("sample.md", Path::new("/watch"), 700), + ] + .into(), + ), + }; + let renders_flag = renders.clone(); + let failures_flag = failures.clone(); + pump( + &rx, + Path::new("/watch"), + Some(OsStr::new("sample.md")), + &mut || { + renders_flag.set(renders_flag.get() + 1); + failures_flag.set(failures_flag.get() + 1); + Err(RenderError::Read { + path: PathBuf::from("/watch/sample.md"), + source: std::io::Error::other("transient"), + }) + }, + Duration::from_millis(200), + &clock, + ); + assert_eq!(renders.get(), 2, "every burst was rendered"); + assert_eq!(failures.get(), 2, "every callback Err surfaced to it"); + } + + #[test] + fn relevance_requires_name_and_parent_match() { + let watch_dir = Path::new("/watch"); + let named = Some(OsStr::new("sample.md")); + + let same_dir = + Event::new(EventKind::Modify(ModifyKind::Any)).add_path(watch_dir.join("sample.md")); + assert!(is_relevant_event(&same_dir, named, watch_dir)); + + // A bare relative path has parent Some("") — never equal to a real + // watch dir, so it is not relevant. Real events carry absolute + // paths, so this only matters to the filter's strictness, not to + // production labeling. + let bare_relative = + Event::new(EventKind::Modify(ModifyKind::Any)).add_path(PathBuf::from("sample.md")); + assert!(!is_relevant_event(&bare_relative, named, watch_dir)); + + let other_dir = Event::new(EventKind::Modify(ModifyKind::Any)) + .add_path(PathBuf::from("/elsewhere/sample.md")); + assert!(!is_relevant_event(&other_dir, named, watch_dir)); + + let other_name = + Event::new(EventKind::Modify(ModifyKind::Any)).add_path(watch_dir.join("other.md")); + assert!(!is_relevant_event(&other_name, named, watch_dir)); + + // No resolvable target: nothing is ever relevant. + let none: Option<&OsStr> = None; + assert!(!is_relevant_event(&same_dir, none, watch_dir)); + } +} diff --git a/tests/cli.rs b/tests/cli.rs index f426c92..124b056 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -838,58 +838,6 @@ fn watch_survives_atomic_rename() { fs::remove_dir_all(dir).expect("failed to clean up temp fixture dir"); } -#[cfg(unix)] -#[test] -fn watch_ignores_sibling_changes() { - use std::process::Stdio; - - let dir = fixture_dir("watch-sibling-changes"); - let input = dir.join("sample.md"); - let output_path = dir.join("sample.html"); - let sibling = dir.join("other.md"); - fs::write(&input, "# Initial\n").expect("failed to write markdown fixture"); - - let child = Command::new(env!("CARGO_BIN_EXE_mdo")) - .arg("--watch") - .arg(&input) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .spawn() - .expect("failed to spawn mdo --watch"); - let _guard = WatchGuard(child); - - assert!( - wait_for_file_containing(&output_path, "Initial", Duration::from_secs(10)), - "initial render should appear within the timeout" - ); - let rendered_before = - fs::read_to_string(&output_path).expect("failed to read initial rendered output"); - let mtime_before = fs::metadata(&output_path) - .and_then(|m| m.modified()) - .expect("failed to read initial rendered output mtime"); - - fs::write(&sibling, "# Sibling\n\nUnrelated file.\n") - .expect("failed to write sibling markdown file"); - std::thread::sleep(Duration::from_secs(1)); - - // A sibling-file event must not trigger a re-render. Byte-comparing the - // output alone is not conclusive — the embedded generated date is - // day-granular and fast renders can format to the same duration — so - // also require the file's mtime (nanosecond granularity on Linux) to be - // untouched: any rewrite, even byte-identical, would bump it. - let rendered_after = fs::read_to_string(&output_path) - .expect("failed to read rendered output after sibling change"); - let mtime_after = fs::metadata(&output_path) - .and_then(|m| m.modified()) - .expect("failed to read rendered output mtime after sibling change"); - assert_eq!( - rendered_before, rendered_after, - "watch mode should not re-render sample.html for changes to an unrelated sibling file" - ); - assert_eq!( - mtime_before, mtime_after, - "sample.html should not have been rewritten (mtime changed) for a sibling-file change" - ); - - fs::remove_dir_all(dir).expect("failed to clean up temp fixture dir"); -} +// Sibling-noise and burst semantics moved into src/watch.rs unit tests +// (fake event channel + virtual clock, cross-platform by construction); +// this file keeps only the spawned-binary e2e smoke test. From 75eabd150e691b246b0a3b7e24c74d8f4c3878cb Mon Sep 17 00:00:00 2001 From: matt wilkie Date: Tue, 6 Oct 2026 23:10:18 -0700 Subject: [PATCH 2/3] Unify the Open-as-HTML handler invocation contract C3 / Beads mdo-6w5.4 (epic mdo-6w5, PR order 4 of 6; stacked on the C2 branch). The fact "the registered handler runs mdo --open and siblings are found beside the running exe" was spelled six times: the Linux desktop Exec template, the Windows handler/registry command variants, mdo-open's forwarding argv per cfg-branch, mdo-setup's two sibling_binary copies, and windows setup's sibling resolution. New src/handler.rs in the lib crate owns the contract: - OPEN_FLAGS + open_argv(): the --open argv prefix spelled once; the file argument itself stays platform presentation (%f placeholder, %1 in a registry command, real path when spawned). - sibling_binary(exe, name): one discovery function, one error mode - a NotFound io error naming BOTH the anchor executable and the path that was checked - so callers print it as-is instead of inventing their own not-found story. - Desktop-entry template, Windows registry command builders, and mdo-open's forward become thin adapters consuming it. Windows-only emission tests now derive their expectation from the contract instead of restating it (2 emitted-form tests: Linux Exec line + Windows registry command), so Linux and Windows cannot drift apart. - mdo-setup's two sibling_binary copies are deleted; the eager is_file check Windows onboarding used to spell inline now applies everywhere through the shared error. In-pass: mdo-open no longer exits silently. current_exe() failure, a missing sibling target in any branch, and spawn failures all print a message before the non-zero exit (a GUI-launched binary may still have no stderr to show it on Windows, but silence is worse everywhere). exit_code_from_status is split into exit_code_from_parts(success, code) so the truncation semantics are unit-testable portably: a failure whose low byte is zero (e.g. 0x100) must not read as success. Covered by four portable tests plus a Windows-only end-to-end (only Windows can produce such a status; the repo's CI runner exercises it). Gates: fmt --check, clippy --all-targets --all-features -D warnings, cargo test --all-targets --all-features --locked all green; the byte-identity harness against the pre-C1 binary still passes for the once-mode CLI surface. Beads-Ref: mdo-6w5.4 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt --- src/bin/mdo-open.rs | 168 ++++++++++++++++++++++++++++++------------- src/bin/mdo-setup.rs | 22 +----- src/file_manager.rs | 78 ++++++++++++++++---- src/handler.rs | 141 ++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + 5 files changed, 330 insertions(+), 80 deletions(-) create mode 100644 src/handler.rs diff --git a/src/bin/mdo-open.rs b/src/bin/mdo-open.rs index df36de0..5cbb1f6 100644 --- a/src/bin/mdo-open.rs +++ b/src/bin/mdo-open.rs @@ -10,10 +10,13 @@ //! but break terminal usage (no stdout to the parent shell, output races the //! returning prompt under cmd/PowerShell). //! -//! Instead, `mdo-open[.exe]` is a tiny desktop wrapper that: -//! 1. Locates `mdo[.exe]` next to itself. -//! 2. Spawns it with `--open` and the rest of the args, using -//! `CREATE_NO_WINDOW` so the child never gets a console allocated. +//! Instead, `mdo-open[.exe]` is a tiny desktop wrapper that consumes the +//! shared handler invocation contract (`mdo_cli::handler`): +//! 1. Locates `mdo[.exe]` next to itself (the one sibling-binary +//! discovery function, so a missing sibling gets one clear message). +//! 2. Spawns it with the contract argv (`--open` plus the file args), +//! using `CREATE_NO_WINDOW` so the child never gets a console +//! allocated. //! 3. If launched directly with no file args, opens onboarding: Windows //! starts the terminal setup in Windows Terminal when available; Linux //! opens `mdo-setup` for native first-run onboarding. This step is @@ -48,8 +51,8 @@ use std::process::{Command, ExitCode}; #[cfg(target_os = "windows")] use std::io; -#[cfg(target_os = "windows")] -use std::path::Path; + +use mdo_cli::handler; #[cfg(target_os = "windows")] const MDO_BIN: &str = "mdo.exe"; @@ -60,12 +63,16 @@ const MDO_BIN: &str = "mdo"; const SETUP_BIN: &str = "mdo-setup"; fn main() -> ExitCode { - let mut exe_path = match env::current_exe() { + // A GUI-launched binary often has no console attached, so this message + // may still go nowhere on Windows — but silent failure is worse + // everywhere, and on Linux it reaches the file manager's stderr log. + let exe_path = match env::current_exe() { Ok(p) => p, - Err(_) => return ExitCode::from(1), + Err(e) => { + eprintln!("mdo-open: cannot locate its own executable: {e}"); + return ExitCode::from(1); + } }; - exe_path.pop(); // strip the file name, keep the directory - let args = env::args_os().skip(1).collect::>(); #[cfg(target_os = "windows")] @@ -77,7 +84,10 @@ fn main() -> ExitCode { if args.is_empty() { return match spawn_windows_onboarding(&exe_path) { Ok(()) => ExitCode::SUCCESS, - Err(_) => ExitCode::from(1), + Err(e) => { + eprintln!("mdo-open: {e}"); + ExitCode::from(1) + } }; } @@ -90,33 +100,47 @@ fn main() -> ExitCode { // instead of always reporting success. match spawn_windows_open(&exe_path, args) { Ok(status) => exit_code_from_status(status), - Err(_) => ExitCode::from(1), + Err(e) => { + eprintln!("mdo-open: {e}"); + ExitCode::from(1) + } } } #[cfg(not(target_os = "windows"))] { #[cfg(target_os = "linux")] - let mut cmd = if args.is_empty() { - let setup = exe_path.join(SETUP_BIN); - if !setup.exists() { - return ExitCode::from(1); + if args.is_empty() { + // No-file launch is onboarding, not open: hand off to the + // sibling `mdo-setup`. + let setup = match handler::sibling_binary(&exe_path, SETUP_BIN) { + Ok(path) => path, + Err(e) => { + eprintln!("mdo-open: {e}"); + return ExitCode::from(1); + } + }; + let mut cmd = Command::new(setup); + match cmd.status() { + Ok(status) => return exit_code_from_status(status), + Err(e) => { + eprintln!("mdo-open: {e}"); + return ExitCode::from(1); + } } - Command::new(setup) - } else { - let mut cmd = Command::new(exe_path.join(MDO_BIN)); - cmd.arg("--open"); - cmd.args(args); - cmd - }; + } - #[cfg(not(target_os = "linux"))] - let mut cmd = { - let mut cmd = Command::new(exe_path.join(MDO_BIN)); - cmd.arg("--open"); - cmd.args(args); - cmd + // Everything else is the forward: sibling mdo with the contract + // argv (`--open` plus the files as received). + let mdo_bin = match handler::sibling_binary(&exe_path, MDO_BIN) { + Ok(path) => path, + Err(e) => { + eprintln!("mdo-open: {e}"); + return ExitCode::from(1); + } }; + let mut cmd = Command::new(mdo_bin); + cmd.args(handler::open_argv(&args)); // Non-Windows binaries never had a console-flash problem to begin // with (that's Windows Explorer + console-subsystem-exe behavior), @@ -126,7 +150,10 @@ fn main() -> ExitCode { // in an already-attached terminal) and propagate its exit status. match cmd.status() { Ok(status) => exit_code_from_status(status), - Err(_) => ExitCode::from(1), + Err(e) => { + eprintln!("mdo-open: {e}"); + ExitCode::from(1) + } } } } @@ -137,10 +164,17 @@ fn main() -> ExitCode { /// a failure whose low byte is zero (e.g. 0x100) must not truncate to /// "success"; any failing status maps to a non-zero code. fn exit_code_from_status(status: std::process::ExitStatus) -> ExitCode { - if status.success() { + exit_code_from_parts(status.success(), status.code()) +} + +/// The truncation rule of [`exit_code_from_status`], split from +/// `ExitStatus` so it is unit-testable portably (Unix cannot even produce a +/// failed status with a zero low byte; Windows can). +fn exit_code_from_parts(success: bool, code: Option) -> ExitCode { + if success { return ExitCode::SUCCESS; } - match status.code() { + match code { Some(code) if code as u8 != 0 => ExitCode::from(code as u8), _ => ExitCode::FAILURE, } @@ -148,28 +182,18 @@ fn exit_code_from_status(status: std::process::ExitStatus) -> ExitCode { #[cfg(target_os = "windows")] fn spawn_windows_open( - exe_dir: &Path, + anchor_exe: &std::path::Path, args: Vec, ) -> io::Result { - let mut cmd = Command::new(exe_dir.join(MDO_BIN)); - cmd.arg("--open"); - cmd.args(args); + let mdo_bin = handler::sibling_binary(anchor_exe, MDO_BIN)?; + let mut cmd = Command::new(mdo_bin); + cmd.args(handler::open_argv(&args)); spawn_without_console(cmd) } #[cfg(target_os = "windows")] -fn spawn_windows_onboarding(exe_dir: &Path) -> io::Result<()> { - let mdo = exe_dir.join(MDO_BIN); - if !mdo.exists() { - return Err(io::Error::new( - io::ErrorKind::NotFound, - format!( - "expected {MDO_BIN} next to mdo-open.exe at {}", - mdo.display() - ), - )); - } - +fn spawn_windows_onboarding(anchor_exe: &std::path::Path) -> io::Result<()> { + let mdo = handler::sibling_binary(anchor_exe, MDO_BIN)?; mdo_cli::windows_setup::spawn_terminal_setup(&mdo) } @@ -189,3 +213,51 @@ fn spawn_without_console(mut cmd: Command) -> io::Result io::Result<()> { continue_after_launcher_registration(register_launcher()); - let mdo = sibling_binary("mdo")?; - if !mdo.exists() { - return Err(io::Error::new( - io::ErrorKind::NotFound, - format!("expected mdo next to mdo-setup at {}", mdo.display()), - )); - } + let mdo = mdo_cli::handler::sibling_binary(&std::env::current_exe()?, "mdo")?; // Already attached to a terminal (e.g. run from a shell): show guided setup // right here instead of spawning a second window. The `?` still @@ -254,12 +248,6 @@ mod linux_setup { .unwrap_or(false) } - fn sibling_binary(name: &str) -> io::Result { - let mut path = std::env::current_exe()?; - path.pop(); - Ok(path.join(name)) - } - #[cfg(test)] mod tests { use super::*; @@ -338,7 +326,7 @@ mod windows_setup { }; pub fn run() -> io::Result<()> { - let mdo = sibling_binary("mdo.exe")?; + let mdo = mdo_cli::handler::sibling_binary(&std::env::current_exe()?, "mdo.exe")?; if !mdo.exists() { return Err(io::Error::new( io::ErrorKind::NotFound, @@ -375,12 +363,6 @@ mod windows_setup { Ok(button) } - fn sibling_binary(name: &str) -> io::Result { - let mut path = std::env::current_exe()?; - path.pop(); - Ok(path.join(name)) - } - fn wide(value: &str) -> Vec { value.encode_utf16().chain(iter::once(0)).collect() } diff --git a/src/file_manager.rs b/src/file_manager.rs index 3674b2f..ddaf02e 100644 --- a/src/file_manager.rs +++ b/src/file_manager.rs @@ -4,6 +4,8 @@ use std::process::Command; #[cfg(target_os = "windows")] use std::process::Stdio; +use crate::handler; + const APP_DISPLAY_NAME: &str = "Open as HTML"; #[cfg(target_os = "linux")] const DESKTOP_FILE_NAME: &str = "mdo.desktop"; @@ -320,13 +322,16 @@ fn xdg_data_home() -> io::Result { #[cfg(target_os = "linux")] fn linux_desktop_entry(exe: &Path) -> String { let quoted_exe = quote_desktop_exec_arg(&exe.to_string_lossy()); + // The Exec line is the handler invocation contract in desktop-entry + // spelling: the binary, the contract flags, then the %f file placeholder. + let open_flags = handler::OPEN_FLAGS.join(" "); format!( "[Desktop Entry]\n\ Type=Application\n\ Name={APP_DISPLAY_NAME}\n\ GenericName=Markdown HTML Opener\n\ Comment=Open Markdown as HTML in the default browser\n\ - Exec={quoted_exe} --open %f\n\ + Exec={quoted_exe} {open_flags} %f\n\ Icon=mdo\n\ Terminal=false\n\ NoDisplay=true\n\ @@ -503,26 +508,32 @@ struct WindowsHandler { #[cfg(target_os = "windows")] fn windows_handler_for(current_exe: &Path) -> WindowsHandler { - let sibling_wrapper = current_exe.with_file_name("mdo-open.exe"); - if sibling_wrapper.exists() { - WindowsHandler { - path: sibling_wrapper, + let sibling_wrapper = handler::sibling_binary(current_exe, "mdo-open.exe").ok(); + match sibling_wrapper { + Some(path) => WindowsHandler { + path, is_wrapper: true, - } - } else { - WindowsHandler { + }, + None => WindowsHandler { path: current_exe.to_path_buf(), is_wrapper: false, - } + }, } } #[cfg(target_os = "windows")] -fn windows_registry_command(handler: &Path, is_wrapper: bool) -> String { +fn windows_registry_command(handler_path: &Path, is_wrapper: bool) -> String { if is_wrapper { - format!("\"{}\" \"%1\"", handler.display()) + format!("\"{}\" \"%1\"", handler_path.display()) } else { - format!("\"{}\" --open \"%1\"", handler.display()) + // The registry command is the handler invocation contract in + // registry spelling: the binary, the contract flags, then the %1 + // file placeholder. + format!( + "\"{}\" {} \"%1\"", + handler_path.display(), + handler::OPEN_FLAGS.join(" ") + ) } } @@ -962,12 +973,55 @@ text/markdown=code.desktop;other.desktop;\n\ image/png=viewer.desktop;\n" ); } + + /// The desktop Exec line is one of the two emitted forms of the handler + /// invocation contract (the other is the Windows registry command): the + /// same binary and the same contract flags, derived from the contract + /// rather than restated as a literal so the two cannot drift apart. + #[test] + fn desktop_entry_reflects_the_handler_invocation_contract() { + let entry = linux_desktop_entry(Path::new("/opt/mdo/mdo")); + let expected = format!( + "Exec=\"/opt/mdo/mdo\" {} %f\n", + handler::OPEN_FLAGS.join(" ") + ); + assert!( + entry.contains(&expected), + "entry should carry the contract Exec line: {entry}" + ); + + // The setup launcher entry is not open-mode: no contract flags. + let setup_entry = linux_setup_desktop_entry(Path::new("/usr/bin/mdo-setup")); + assert!(!setup_entry.contains(handler::OPEN_FLAGS[0])); + } } #[cfg(all(test, target_os = "windows"))] mod windows_tests { use super::*; + /// The registry command is one of the two emitted forms of the handler + /// invocation contract (the other is the Linux desktop Exec line): it + /// must carry the same binary and the same contract flags, derived here + /// rather than restated as a literal so the two cannot drift apart. + #[test] + fn registry_command_reflects_the_handler_invocation_contract() { + let command = windows_registry_command(Path::new(r"C:\Tools\mdo.exe"), false); + let expected = format!( + r#""C:\Tools\mdo.exe" {} "%1""#, + handler::OPEN_FLAGS.join(" ") + ); + assert_eq!(command, expected); + + // The forwarding wrapper carries NO flags: it owns the contract + // itself and forwards the file as received. + let wrapper = windows_registry_command(Path::new(r"C:\Tools\mdo-open.exe"), true); + assert_eq!(wrapper, r#""C:\Tools\mdo-open.exe" "%1""#); + assert!(!wrapper.contains(handler::OPEN_FLAGS[0])); + } + + use super::*; + #[test] fn registry_command_uses_open_flag_without_wrapper() { let command = windows_registry_command(Path::new(r"C:\Tools\mdo.exe"), false); diff --git a/src/handler.rs b/src/handler.rs new file mode 100644 index 0000000..e879228 --- /dev/null +++ b/src/handler.rs @@ -0,0 +1,141 @@ +//! The Open-as-HTML handler invocation contract. +//! +//! Every platform surface that registers or forwards Open as HTML spells +//! the same two facts — how a handler launches mdo (`--open` plus the +//! file), and how mdo's binaries are found beside each other (siblings of +//! the running executable). This module owns both; the Linux desktop-entry +//! template, the Windows registry command builders, and the `mdo-open` +//! forwarding binary are thin adapters consuming them, so a change to the +//! invocation is made here once and every platform entry stays in step. +//! +//! The file argument itself is platform presentation: a `%f` placeholder in +//! a desktop entry, `%1` in a Windows registry command string, and the real +//! path when a binary is spawned. Only the argv prefix is the contract. + +use std::ffi::OsString; +use std::io; +use std::path::{Path, PathBuf}; + +/// The flags a handler command puts before the file argument when it +/// launches mdo directly. Spelled once: the desktop-entry template, the +/// Windows registry command, and the forward binary all consume this, so +/// the contract cannot drift between the six places that used to repeat it. +pub const OPEN_FLAGS: &[&str] = &["--open"]; + +/// The full open-mode argv a handler spawns mdo with: the contract flags +/// (see [`OPEN_FLAGS`]) followed by the files to open, in order. +pub fn open_argv(files: &[OsString]) -> Vec { + let mut argv = Vec::with_capacity(OPEN_FLAGS.len() + files.len()); + argv.extend(OPEN_FLAGS.iter().map(OsString::from)); + argv.extend(files.iter().cloned()); + argv +} + +/// Locate a sibling binary beside the running one (or beside whichever +/// executable the caller anchors on). This is the one discovery function +/// and the one error mode for everything mdo ships: the binaries always +/// run from the same directory. +/// +/// The error is a `NotFound` io error naming the anchor and the path that +/// was checked, so callers can print it as-is instead of inventing their +/// own "not found" story. +pub fn sibling_binary(exe: &Path, name: &str) -> io::Result { + let parent = exe.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + format!( + "no parent directory for {} (looking for {name})", + exe.display() + ), + ) + })?; + let sibling = parent.join(name); + if !sibling.is_file() { + return Err(io::Error::new( + io::ErrorKind::NotFound, + format!( + "no {name} found beside {} (looked at {})", + exe.display(), + sibling.display() + ), + )); + } + Ok(sibling) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + + fn fixture_dir(name: &str) -> PathBuf { + let uuid = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system time should be after Unix epoch") + .as_nanos(); + let dir = std::env::temp_dir().join(format!( + "mdo-handler-test-{name}-{}-{uuid}", + std::process::id() + )); + fs::create_dir_all(&dir).expect("failed to create temp fixture dir"); + dir + } + + #[test] + fn open_argv_prepends_the_contract_flags_before_the_files() { + let files = vec![OsString::from("notes.md"), OsString::from("todo.md")]; + let argv = open_argv(&files); + let argv: Vec = argv + .iter() + .map(|arg| arg.to_string_lossy().into_owned()) + .collect(); + assert_eq!(argv[0], OPEN_FLAGS[0], "the contract flag comes first"); + assert_eq!(argv[1..], ["notes.md", "todo.md"]); + } + + #[test] + fn sibling_binary_resolves_a_real_sibling() { + let dir = fixture_dir("resolves"); + let anchor = dir.join("mdo"); + fs::write(&anchor, "binary").expect("failed to write anchor fixture"); + let sibling = dir.join("mdo-open"); + fs::write(&sibling, "binary").expect("failed to write sibling fixture"); + + let found = sibling_binary(&anchor, "mdo-open") + .expect("a real sibling beside the anchor should resolve"); + assert_eq!(found, sibling); + } + + #[test] + fn sibling_binary_error_names_anchor_and_looked_at_path() { + let dir = fixture_dir("missing"); + let anchor = dir.join("mdo"); + fs::write(&anchor, "binary").expect("failed to write anchor fixture"); + + let err = sibling_binary(&anchor, "mdo-open") + .expect_err("a missing sibling is the one error mode"); + assert_eq!(err.kind(), io::ErrorKind::NotFound); + let message = err.to_string(); + assert!( + message.contains(&anchor.display().to_string()), + "error should name the anchor: {message}" + ); + assert!( + message.contains(&dir.join("mdo-open").display().to_string()), + "error should name the path that was checked: {message}" + ); + } + + #[test] + fn sibling_binary_rejects_a_directory_named_like_a_binary() { + let dir = fixture_dir("not-a-file"); + let anchor = dir.join("mdo"); + fs::write(&anchor, "binary").expect("failed to write anchor fixture"); + let dir_sibling = dir.join("mdo-open"); + fs::create_dir_all(&dir_sibling).expect("failed to create decoy dir"); + + let err = + sibling_binary(&anchor, "mdo-open").expect_err("a directory is not a runnable sibling"); + assert_eq!(err.kind(), io::ErrorKind::NotFound); + } +} diff --git a/src/lib.rs b/src/lib.rs index c962aed..b3b102f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -23,6 +23,7 @@ use url::Url; #[cfg(target_os = "android")] mod android; pub mod file_manager; +pub mod handler; mod highlight; pub mod watch; #[cfg(target_os = "windows")] From 2239ee71eb18f4c451d0f6b5e62d00dc2664f745 Mon Sep 17 00:00:00 2001 From: matt wilkie Date: Tue, 6 Oct 2026 23:17:34 -0700 Subject: [PATCH 3/3] Shape Linux integration install like the Windows plan/executor C5 / Beads mdo-6w5.5 (epic mdo-6w5, PR order 5 of 6; stacked on the C3 branch). Mirror image for the Linux side of the Windows shape the code already has (pure plan + thin executor + typed results): - linux_install_plan(data_home, current_exe, set_default) is a PURE function: the desktop-file/icon targets, the entry content (which now shows it carries the handler invocation contract), the default mimes, and the legacy-cleanup root - unit-testable without spawn-capture. - apply_linux_install_plan(plan) is the only file-touching step; it returns LinuxInstallReport (paths written + LinuxDefaultOutcome: Applied / NeedsManualHint / NotRequested) instead of reporting through stdout mid-apply. - print_linux_install_report(report) is the executor-caller's CLI surface; the exact lines mirror the pre-plan flow byte for byte (verified: install output diffed against the pre-epic binary modulo the fixture paths; both exit 0). - The three _impl trampolines (install/uninstall/integration_installed across linux/windows/not-any cfg bodies) are collapsed into the pub fns directly, on every platform including the not(any) fallbacks. - Dead pubs removed: install_linux_for_exe and install_windows_for_exe had zero external callers; the Windows body moves into install() verbatim (only the handler locals rename to avoid the new `handler` module import shadow). Doc references updated. SCOPE LINE per ADR-0004 (Deferred): strictly the v0.6 minimal owned-registration check - no repair flows, no remnant cleanup, no partial-state classification; reopening ADR-0004 is out of scope. The scripts/uninstall-*.legacy helper scripts are a known duplication but explicitly out of scope; a retirement decision would be its own ADR. Gates: fmt --check, clippy --all-targets --all-features -D warnings, cargo test --all-targets --all-features --locked all green (3 new Linux plan/executor tests; Windows-side tests unchanged and still asserting the registry plan byte-exactly). Beads-Ref: mdo-6w5.5 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt --- src/file_manager.rs | 486 ++++++++++++++++++++++++++++++-------------- 1 file changed, 330 insertions(+), 156 deletions(-) diff --git a/src/file_manager.rs b/src/file_manager.rs index ddaf02e..8140994 100644 --- a/src/file_manager.rs +++ b/src/file_manager.rs @@ -29,103 +29,284 @@ const SVG_ICON: &str = r##"