From 551873767d096f5f77229da39a114b43fa489000 Mon Sep 17 00:00:00 2001 From: kronberger-droid Date: Wed, 19 Aug 2026 19:04:09 +0200 Subject: [PATCH 1/2] refactor(engine): propagate history errors instead of expect("todo: error handling") `impl From for io::Error` (unwrapping the `IOError` variant so the kind survives) lets the nine history sites in `engine.rs` use `?`. `run_history_commands`, `previous_history`, `next_history`, `up_command` and `down_command` grow `io::Result<()>`; all their callers already sit in `io::Result` fns, so `read_line` now returns `Err` where it used to panic. `history.save` in `submit_buffer` is left for the next commit, since failing there would lose the submitted command. --- src/engine.rs | 94 ++++++++++++++++++++++----------------------------- src/result.rs | 9 +++++ 2 files changed, 50 insertions(+), 53 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index d1916b86..45940890 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -814,8 +814,7 @@ impl Reedline { pub fn print_history(&mut self) -> Result<()> { let history: Vec<_> = self .history - .search(SearchQuery::everything(SearchDirection::Forward, None)) - .expect("todo: error handling"); + .search(SearchQuery::everything(SearchDirection::Forward, None))?; for (i, entry) in history.iter().enumerate() { self.print_line(&format!("{}\t{}", i, entry.command_line))?; @@ -825,13 +824,10 @@ impl Reedline { /// Output the complete [`History`] for this session, chronologically with numbering to the terminal pub fn print_history_session(&mut self) -> Result<()> { - let history: Vec<_> = self - .history - .search(SearchQuery::everything( - SearchDirection::Forward, - self.get_history_session_id(), - )) - .expect("todo: error handling"); + let history: Vec<_> = self.history.search(SearchQuery::everything( + SearchDirection::Forward, + self.get_history_session_id(), + ))?; for (i, entry) in history.iter().enumerate() { self.print_line(&format!("{}\t{}", i, entry.command_line))?; @@ -1308,7 +1304,7 @@ impl Reedline { self.editor.reset_undo_stack(); Ok(EventStatus::Exits(Signal::CtrlD)) } else { - self.run_history_commands(&[EditCommand::Delete]); + self.run_history_commands(&[EditCommand::Delete])?; Ok(EventStatus::Handled) } } @@ -1342,7 +1338,7 @@ impl Reedline { Ok(EventStatus::Exits(Signal::HostCommand(host_command))) } ReedlineEvent::Edit(commands) => { - self.run_history_commands(&commands); + self.run_history_commands(&commands)?; Ok(EventStatus::Handled) } ReedlineEvent::Mouse { @@ -1365,20 +1361,14 @@ impl Reedline { Ok(EventStatus::Handled) } ReedlineEvent::PreviousHistory | ReedlineEvent::Up | ReedlineEvent::SearchHistory => { - self.history_cursor - .back(self.history.as_ref()) - .expect("todo: error handling"); + self.history_cursor.back(self.history.as_ref())?; Ok(EventStatus::Handled) } ReedlineEvent::NextHistory | ReedlineEvent::Down => { - self.history_cursor - .forward(self.history.as_ref()) - .expect("todo: error handling"); + self.history_cursor.forward(self.history.as_ref())?; // Hacky way to ensure that we don't fall of into failed search going forward if self.history_cursor.string_at_cursor().is_none() { - self.history_cursor - .back(self.history.as_ref()) - .expect("todo: error handling"); + self.history_cursor.back(self.history.as_ref())?; } Ok(EventStatus::Handled) } @@ -1703,19 +1693,19 @@ impl Reedline { Ok(EventStatus::Handled) } ReedlineEvent::PreviousHistory => { - self.previous_history(); + self.previous_history()?; Ok(EventStatus::Handled) } ReedlineEvent::NextHistory => { - self.next_history(); + self.next_history()?; Ok(EventStatus::Handled) } ReedlineEvent::Up => { - self.up_command(); + self.up_command()?; Ok(EventStatus::Handled) } ReedlineEvent::Down => { - self.down_command(); + self.down_command()?; Ok(EventStatus::Handled) } ReedlineEvent::Left => { @@ -1855,7 +1845,7 @@ impl Reedline { .for_each(|menu| menu.menu_event(MenuEvent::Deactivate)); } - fn previous_history(&mut self) { + fn previous_history(&mut self) -> io::Result<()> { self.history_cursor_on_excluded = false; if self.input_mode != InputMode::HistoryTraversal { self.input_mode = InputMode::HistoryTraversal; @@ -1870,9 +1860,8 @@ impl Reedline { } if !self.history_cursor_on_excluded { - self.history_cursor - .back(self.history.as_ref()) - .expect("todo: error handling"); + // On `Err` the next press retries on the fresh cursor; no rollback. + self.history_cursor.back(self.history.as_ref())?; } self.update_buffer_from_history(); self.editor.move_to_start(false); @@ -1882,9 +1871,10 @@ impl Reedline { self.editor.commit_cursor(); self.editor .update_undo_state(UndoBehavior::HistoryNavigation); + Ok(()) } - fn next_history(&mut self) { + fn next_history(&mut self) -> io::Result<()> { if self.input_mode != InputMode::HistoryTraversal { self.input_mode = InputMode::HistoryTraversal; self.history_cursor = HistoryCursor::new( @@ -1897,9 +1887,7 @@ impl Reedline { self.history_cursor_on_excluded = false; } else { let cursor_was_on_item = self.history_cursor.string_at_cursor().is_some(); - self.history_cursor - .forward(self.history.as_ref()) - .expect("todo: error handling"); + self.history_cursor.forward(self.history.as_ref())?; if cursor_was_on_item && self.history_cursor.string_at_cursor().is_none() @@ -1917,7 +1905,8 @@ impl Reedline { // See `previous_history`: settle the out-of-band cursor under the policy. self.editor.commit_cursor(); self.editor - .update_undo_state(UndoBehavior::HistoryNavigation) + .update_undo_state(UndoBehavior::HistoryNavigation); + Ok(()) } /// Enable the search and navigation through the history from the line buffer prompt @@ -1955,7 +1944,7 @@ impl Reedline { /// Dispatches the applicable [`EditCommand`] actions for editing the history search string. /// /// Only modifies internal state, does not perform regular output! - fn run_history_commands(&mut self, commands: &[EditCommand]) { + fn run_history_commands(&mut self, commands: &[EditCommand]) -> io::Result<()> { for command in commands { match command { EditCommand::InsertChar(c) => { @@ -1972,9 +1961,7 @@ impl Reedline { self.get_history_session_id(), ); } - self.history_cursor - .back(self.history.as_mut()) - .expect("todo: error handling"); + self.history_cursor.back(self.history.as_mut())?; } EditCommand::Backspace => { let navigation = self.history_cursor.get_navigation(); @@ -1986,9 +1973,7 @@ impl Reedline { HistoryNavigationQuery::SubstringSearch(new_substring.to_string()), self.get_history_session_id(), ); - self.history_cursor - .back(self.history.as_mut()) - .expect("todo: error handling"); + self.history_cursor.back(self.history.as_mut())? } } _ => { @@ -1996,6 +1981,7 @@ impl Reedline { } } } + Ok(()) } /// Set the buffer contents for history traversal/search in the standard prompt @@ -2057,27 +2043,29 @@ impl Reedline { } } - fn up_command(&mut self) { + fn up_command(&mut self) -> io::Result<()> { // If we're at the top, then: if self.editor.is_cursor_at_first_line() { // If we're at the top, move to previous history - self.previous_history(); + self.previous_history() } else { // Through `apply_edit_commands` so the cursor settles under the mode's // rest policy — a bare `editor.move_line_up` skips the commit boundary, // leaving a vi-normal caret past the last grapheme on a short line. self.apply_edit_commands(&[EditCommand::MoveLineUp { select: false }]); + Ok(()) } } - fn down_command(&mut self) { + fn down_command(&mut self) -> io::Result<()> { // If we're at the top, then: if self.editor.is_cursor_at_last_line() { // If we're at the top, move to previous history - self.next_history(); + self.next_history() } else { // See `up_command`: settle under the rest policy via the commit boundary. self.apply_edit_commands(&[EditCommand::MoveLineDown { select: false }]); + Ok(()) } } @@ -2850,7 +2838,7 @@ mod tests { .expect("Failed to save history"); // Navigate to previous history - reedline.previous_history(); + reedline.previous_history().expect("history ok"); // Get the initial insertion point after history navigation let initial_insertion_point = reedline.current_insertion_point(); @@ -3958,7 +3946,7 @@ mod tests { let history = HistoryItem::from_command_line(input); reedline.history.save(history).unwrap(); - reedline.previous_history(); + reedline.previous_history().expect("history ok"); let move_to_start = EditCommand::MoveToLineStart { select: false }; reedline.run_edit_commands(&[move_to_start]); @@ -3997,7 +3985,7 @@ mod tests { let history = HistoryItem::from_command_line(input); reedline.history.save(history).unwrap(); - reedline.previous_history(); + reedline.previous_history().expect("history ok"); let move_to_start = EditCommand::MoveToLineStart { select: false }; reedline.run_edit_commands(&[move_to_start]); @@ -4020,9 +4008,9 @@ mod tests { let history = HistoryItem::from_command_line(input); reedline.history.save(history).unwrap(); - reedline.previous_history(); + reedline.previous_history().expect("history ok"); - reedline.down_command(); + reedline.down_command().expect("history ok"); let move_to_start = EditCommand::MoveToLineStart { select: false }; reedline.run_edit_commands(&[move_to_start]); @@ -4048,7 +4036,7 @@ mod tests { let history = HistoryItem::from_command_line(input); reedline.history.save(history).unwrap(); - reedline.previous_history(); + reedline.previous_history().expect("history ok"); let move_to_end = EditCommand::MoveToEnd { select: false }; reedline.run_edit_commands(&[move_to_end]); @@ -4076,7 +4064,7 @@ mod tests { // Save "6" to the history and scroll back to it let history = HistoryItem::from_command_line("6"); reedline.history.save(history).unwrap(); - reedline.previous_history(); + reedline.previous_history().expect("history ok"); assert_eq!(reedline.current_buffer_contents(), "6"); // Perform quick completion @@ -4603,7 +4591,7 @@ mod tests { EditCommand::MoveRight { select: false }, EditCommand::MoveRight { select: false }, ]); // caret on 'c' (col 2 of line 1) - rl.down_command(); + rl.down_command().expect("history ok"); assert_eq!(rl.editor.insertion_point(), 4); // on 'd', not 5 (past it) } @@ -4629,7 +4617,7 @@ mod tests { let mut rl = seam_engine(Box::::default()); rl.run_edit_commands(&[EditCommand::InsertString("abc".into())]); drive(&mut rl, &[key(KeyCode::Esc)]); // vi normal, on 'c' - rl.down_command(); // last line -> next_history (no forward entry -> draft) + rl.down_command().expect("history ok"); // last line -> next_history (no forward entry -> draft) assert_eq!(rl.editor.insertion_point(), 2); // 'c', not 3 (past it) } diff --git a/src/result.rs b/src/result.rs index 45fad917..5fe0ff2b 100644 --- a/src/result.rs +++ b/src/result.rs @@ -48,3 +48,12 @@ impl std::error::Error for ReedlineError {} /// Standard [`std::result::Result`], with [`ReedlineError`] as the error variant pub type Result = std::result::Result; + +impl From for std::io::Error { + fn from(err: ReedlineError) -> Self { + match err.0 { + ReedlineErrorVariants::IOError(io) => io, + other => std::io::Error::other(ReedlineError(other)), + } + } +} From b661a05aec1cedb151ee43ac6523f6c70d563075 Mon Sep 17 00:00:00 2001 From: kronberger-droid Date: Wed, 19 Aug 2026 19:05:38 +0200 Subject: [PATCH 2/2] feat(engine): keep the submitted line when the history save fails `submit_buffer` was the last `expect("todo: error handling")` and the one where `?` is wrong: `read_line` returning `Err` would drop the command the user just typed over a history write. Instead the entry is treated like an excluded one (`FILTERED_ITEM_ID`, `history_excluded_item`), so Up still recalls it and `update_last_command_context` updates it in memory, and the error is stashed for `Reedline::take_history_save_error`. Additive API; cleared on read, set at most once per `read_line`. --- src/engine.rs | 128 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 118 insertions(+), 10 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 45940890..f34b4955 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -144,6 +144,8 @@ pub struct Reedline { history_exclusion_prefix: Option, history_excluded_item: Option, history_cursor_on_excluded: bool, + /// Last failed `history.save`, until [`Reedline::take_history_save_error`]. + history_save_error: Option, input_mode: InputMode, // State of the painter after a `ReedlineEvent::ExecuteHostCommand` was requested, used after @@ -357,6 +359,7 @@ impl Reedline { history_exclusion_prefix: None, history_excluded_item: None, history_cursor_on_excluded: false, + history_save_error: None, input_mode: InputMode::Regular, suspended_state: None, last_render_snapshot: None, @@ -894,6 +897,15 @@ impl Reedline { } } + /// Take the error of the last failed history save, if any. + /// + /// [`read_line`](Self::read_line) still returns the line when the [`History`] + /// refuses to store it; the entry is then treated like an excluded one. + /// Cleared on read, set at most once per `read_line`. + pub fn take_history_save_error(&mut self) -> Option { + self.history_save_error.take() + } + /// Wait for input and provide the user with a specified [`Prompt`]. /// /// Returns a [`std::io::Result`] in which the `Err` type is [`std::io::Result`] @@ -2657,19 +2669,35 @@ impl Reedline { let mut entry = HistoryItem::from_command_line(&buffer); entry.session_id = self.get_history_session_id(); - if self + let excluded = self .history_exclusion_prefix .as_ref() - .map(|prefix| buffer.starts_with(prefix)) - .unwrap_or(false) - { - entry.id = Some(Self::FILTERED_ITEM_ID); - self.history_last_run_id = entry.id; - self.history_excluded_item = Some(entry); + .is_some_and(|prefix| buffer.starts_with(prefix)); + + let saved = if excluded { + None } else { - entry = self.history.save(entry).expect("todo: error handling"); - self.history_last_run_id = entry.id; - self.history_excluded_item = None; + match self.history.save(entry.clone()) { + Ok(saved) => Some(saved), + Err(err) => { + // Ran but not stored: the excluded shape. Keep the line, + // stash the error for `take_history_save_error`. + self.history_save_error = Some(err); + None + } + } + }; + + match saved { + Some(saved) => { + self.history_last_run_id = saved.id; + self.history_excluded_item = None; + } + None => { + entry.id = Some(Self::FILTERED_ITEM_ID); + self.history_last_run_id = entry.id; + self.history_excluded_item = Some(entry); + } } } self.run_edit_commands(&[EditCommand::Clear]); @@ -2917,6 +2945,86 @@ mod tests { ); } + // --- a history that refuses to save --- + + struct RefusingHistory; + + impl History for RefusingHistory { + fn save(&mut self, _h: HistoryItem) -> crate::Result { + Err(ReedlineError(ReedlineErrorVariants::OtherHistoryError( + "refused", + ))) + } + fn load(&self, _id: HistoryItemId) -> crate::Result { + unreachable!("not used") + } + fn count(&self, _query: SearchQuery) -> crate::Result { + Ok(0) + } + fn search(&self, _query: SearchQuery) -> crate::Result> { + Ok(vec![]) + } + fn update( + &mut self, + _id: HistoryItemId, + _updater: &dyn Fn(HistoryItem) -> HistoryItem, + ) -> crate::Result<()> { + unreachable!("not used") + } + fn clear(&mut self) -> crate::Result<()> { + Ok(()) + } + fn delete(&mut self, _h: HistoryItemId) -> crate::Result<()> { + Ok(()) + } + fn sync(&mut self) -> std::io::Result<()> { + Ok(()) + } + fn session(&self) -> Option { + None + } + } + + fn refusing_history_engine() -> Reedline { + let mut rl = seam_engine(Box::::default()).with_history(Box::new(RefusingHistory)); + rl.painter.force_prompt_anchored_for_test(0); + rl + } + + #[test] + fn failed_history_save_still_returns_the_line_and_stashes_the_error() { + let mut rl = refusing_history_engine(); + let signal = drive_until_signal(&mut rl, &[ch('l'), ch('s'), key(KeyCode::Enter)]); + assert!( + matches!(signal, Some(Signal::Success(ref s)) if s == "ls"), + "got {signal:?}" + ); + let err = rl.take_history_save_error(); + assert!(err.is_some(), "the save error is stashed"); + assert!(rl.take_history_save_error().is_none(), "cleared on read"); + } + + #[test] + fn failed_history_save_keeps_the_entry_reachable() { + let mut rl = refusing_history_engine(); + drive_until_signal(&mut rl, &[ch('l'), ch('s'), key(KeyCode::Enter)]); + + drive(&mut rl, &[key(KeyCode::Up)]); + assert_eq!(rl.editor.get_buffer(), "ls", "Up recalls the unsaved entry"); + + rl.update_last_command_context(&|mut item| { + item.exit_status = Some(7); + item + }) + .expect("context update works off the store"); + assert_eq!( + rl.history_excluded_item + .as_ref() + .and_then(|i| i.exit_status), + Some(7) + ); + } + #[test] fn thread_safe() { fn f(_: S) {}