From 49c61d15f9a12d6030bb000cd28fdb3693990dc2 Mon Sep 17 00:00:00 2001 From: Steffen Dienst Date: Fri, 25 Sep 2026 12:36:21 +0200 Subject: [PATCH 1/2] fix(nrepl): stop the harness freezing when an eval errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed eval was treated as a dead socket. `nrepl-eval` wrapped the whole eval in a retry that reconnected and re-ran it on ANY error: (try (nrepl-eval-inner repaired) ([err] (nrepl-reconnect) (nrepl-eval-inner repaired))) Two bad outcomes followed. For a TIMEOUT the slow work was simply run again, doubling the wait (a 3 s timeout took 6 s). Worse, the retry reconnects first: `connect-nrepl-inner` did an unbounded blocking read for the clone reply, and when the server had gone away that read never returned. A blocking C socket read is not reachable by the Janet interrupt, so dirge's eval/interrupt machinery could not stop it — the worker sat there until the host's own deadline and the session froze completely. Fix, in three parts: - `nrepl-connection-error?` retries only on genuine transport failures (broken pipe / closed socket / connection reset / EOF / not connected). An ordinary eval error — a compile error, a thrown exception — now propagates with the connection intact. - A timeout is not retried. The eval keeps running server-side after the timeout, so its late reply would desync the next eval; the connection is dropped instead (`nrepl-disconnect`) and the next `nrepl_eval` call reconnects via its existing `.nrepl-port` path. `nrepl-timeout-error?` gates this. - The clone handshake is bounded by a new `nrepl-connect-timeout` (10 s, deliberately independent of `nrepl-eval-timeout`, which may be minutes for a long computation). An endpoint that accepts the TCP connection but never answers now fails fast with "nREPL clone handshake failed: timeout" instead of wedging. `nrepl-read-msg` already took an optional timeout; the handshake simply passes it now. Regression tests (`cargo test --features plugin nrepl`): - `nrepl_timeout_is_not_retried_as_a_connection_error` — timeout text must not be classified as a retryable connection error; transport failures must. - `nrepl_timeout_detection_is_precise` — only timeout wording may drop the connection; a compile error or thrown exception must leave it intact. Verified live against a real REPL at 127.0.0.1:8230: a 3 s timeout now returns in 3 s (previously 6 s), after which the connection reports "not connected"; a silent-accept server fails the connect in ~3 s instead of hanging past the 30 s worker deadline (the freeze being fixed). --- CHANGELOG.md | 15 ++++++++ plugins/nrepl/00-state.janet | 69 +++++++++++++++++++++++++++++++--- plugins/nrepl/README.md | 9 +++++ src/plugin/mod_tests.rs | 73 ++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34cb0c4b6..3eef873d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,21 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- The nREPL plugin no longer hangs the harness when an eval errors. A failed + eval used to be treated as a dead socket: `nrepl-eval` reconnected and + re-ran the *entire* eval on any error. For a timeout that doubled the wait + (a 3 s timeout took 6 s) and, when the server had gone away, turned the + retry into an unbounded clone handshake — a blocking socket read the Janet + interrupt cannot reach, which froze dirge until its own 30 s worker + deadline. Now only genuine transport failures (broken pipe / closed socket + / connection reset) trigger the reconnect-and-retry; a timeout drops the + connection instead (the timed-out eval keeps running server-side, so its + late reply would desync the next eval), and any other eval error just + propagates with the connection intact. The clone handshake is bounded by + `nrepl-connect-timeout` (10 s), so a server that accepts but never answers + fails fast instead of wedging. + ## [0.26.0] - 2026-10-26 ### Added diff --git a/plugins/nrepl/00-state.janet b/plugins/nrepl/00-state.janet index 7360c8050..a1d4da27c 100644 --- a/plugins/nrepl/00-state.janet +++ b/plugins/nrepl/00-state.janet @@ -105,6 +105,7 @@ (var nrepl-port nil) (var nrepl-connected false) (var nrepl-eval-timeout 120) # per-eval timeout in seconds +(var nrepl-connect-timeout 10) # seconds to await the clone handshake (var nrepl-current-eval-id nil) # active eval id for interrupt (var nrepl-rbuf @"") # bytes read but not yet decoded (see below) @@ -154,7 +155,16 @@ # Fresh socket → drop any leftover bytes from a previous session. (set nrepl-rbuf @"") (nrepl-send-msg conn @{"op" "clone" "id" "dirge-clone"}) - (def clone-resp (nrepl-read-msg conn)) + # Bounded: an endpoint that accepts but never answers must fail, not + # wedge the (uninterruptible) C read. The bound is deliberately + # independent of `nrepl-eval-timeout` — that one may be minutes for a + # long computation, which would be an unacceptable connect hang. + (def clone-resp + (try + (nrepl-read-msg conn nrepl-connect-timeout) + ([err] + (try (:close conn) ([_] nil)) + (error (string "nREPL clone handshake failed: " err))))) (def session (get clone-resp "new-session")) (set nrepl-conn conn) (set nrepl-session session) @@ -203,6 +213,38 @@ (set nrepl-rbuf @"")) (connect-nrepl-inner nrepl-host nrepl-port)) +(defn nrepl-connection-error? [err] + "True only for errors that mean the SOCKET is gone and a reconnect + + retry can help. An eval error (including the per-eval timeout) is not + one: retrying it repeats work the user is already waiting on, and if + the server died mid-eval the retry's clone handshake blocks in a C + socket read that no interrupt can reach — freezing dirge until the + host's own deadline. So match on transport-failure text only." + (def s (string/ascii-lower (string err))) + (def needles + ["connection closed by server" "broken pipe" "connection reset" + "connection aborted" "eof" "not connected"]) + (var hit false) + (each needle needles + (if (string/find needle s) + (set hit true))) + hit) + +(defn nrepl-timeout-error? [err] + "True when an eval failed on the per-eval timeout (either our own + \"timed out after Ns\" or the Janet read timeout). The connection has + to be dropped in that case: the eval keeps running server-side and its + late reply would desync the next eval. Matches on timeout wording only, + so an ordinary eval error (compile error, thrown exception) leaves the + connection intact." + (def s (string/ascii-lower (string err))) + (def needles ["timeout" "timed out"]) + (var hit false) + (each needle needles + (if (string/find needle s) + (set hit true))) + hit) + # ── paren repair ───────────────────────────────────────────────── # # LLMs frequently emit Clojure code with unbalanced delimiters. @@ -337,11 +379,26 @@ (var result (try (nrepl-eval-inner repaired) ([err] - # A broken-pipe / closed error means the socket went stale - # (e.g. the nREPL server was restarted). Reconnect once and - # retry; if the server is genuinely down, this re-raises. - (nrepl-reconnect) - (nrepl-eval-inner repaired)))) + (cond + # A transport failure (broken pipe / closed socket) means the + # socket went stale (e.g. the nREPL server was restarted). + # Reconnect once and retry then. + (nrepl-connection-error? err) + (do + (nrepl-reconnect) + (nrepl-eval-inner repaired)) + # A TIMEOUT is not retried: the slow eval would just run + # again (doubling the wait) and, if the server is gone, the + # retry's clone handshake wedges dirge. Worse, the timed-out + # eval is still running server-side, so its late reply would + # desync the next eval — drop the connection to start clean. + (nrepl-timeout-error? err) + (do + (nrepl-disconnect) + (error err)) + # Any other eval error (compile error, thrown exception) + # leaves the connection perfectly usable — propagate it. + (error err))))) (def result-table @{:result (get result "result") :out (get result "out") :err (get result "err") diff --git a/plugins/nrepl/README.md b/plugins/nrepl/README.md index 3e35eaa0e..2f8c2440e 100644 --- a/plugins/nrepl/README.md +++ b/plugins/nrepl/README.md @@ -33,6 +33,15 @@ attempt. Strings and line comments are skipped. `/nrepl-timeout`). An `interrupt` op is sent when the limit is exceeded. `/nrepl-interrupt` is available for manual cancellation. +A timed-out eval is *not* retried. The eval keeps running server-side after +the timeout, so its late reply would desync every later eval; instead the +connection is dropped, and the next `nrepl_eval` reconnects cleanly. Only a +genuine transport failure (broken pipe / closed socket) triggers the +reconnect-and-retry path — an ordinary eval error (compile error, thrown +exception) just propagates with the connection intact. The connect handshake +is also bounded (`nrepl-connect-timeout`, 10 s) so a server that accepts the +TCP connection but never answers fails fast instead of hanging the harness. + ## Installation Copy into one of two auto-discovered directories: diff --git a/src/plugin/mod_tests.rs b/src/plugin/mod_tests.rs index 93f8a5bc1..5dd22d0f0 100644 --- a/src/plugin/mod_tests.rs +++ b/src/plugin/mod_tests.rs @@ -3943,3 +3943,76 @@ fn top_dyns_offset_is_stable() { "janet_dyn must read back what janet_setdyn wrote" ); } + +/// An eval that times out must NOT be treated as a dead connection. +/// `nrepl-eval`'s retry path reconnects and re-runs the whole eval on any +/// error; for a *timeout* that just repeats the slow work (observed: a 3 s +/// timeout took 6 s) and, when the server has gone away, turns a clean +/// timeout into an uninterruptible handshake that freezes dirge. So only +/// genuine transport failures may be retried. +#[cfg(feature = "plugin")] +#[test] +fn nrepl_timeout_is_not_retried_as_a_connection_error() { + let mut mgr = nrepl_state_env(); + for msg in [ + "nREPL eval timed out after 3s", + "timeout", + "Runtime VM error: timeout", + ] { + assert_eq!( + mgr.eval(&format!("(nrepl-connection-error? \"{msg}\")")) + .unwrap(), + "false", + "{msg:?} must not be retried" + ); + } + for msg in [ + "nREPL connection closed by server", + "broken pipe", + "Connection reset by peer", + ] { + assert_eq!( + mgr.eval(&format!("(nrepl-connection-error? \"{msg}\")")) + .unwrap(), + "true", + "{msg:?} must be retried" + ); + } +} + +/// `nrepl-timeout-error?` gates the OTHER half of the timeout handling: +/// after a timeout the eval is still running server-side and its eventual +/// reply would desync every later eval, so `nrepl-eval` must drop the +/// connection rather than reuse the session. Only timeout text may do that; +/// an ordinary eval error (a compile error, a thrown exception) leaves the +/// connection perfectly usable and must NOT be torn down. +#[cfg(feature = "plugin")] +#[test] +fn nrepl_timeout_detection_is_precise() { + let mut mgr = nrepl_state_env(); + for msg in [ + "nREPL eval timed out after 3s", + "timeout", + "Runtime VM error: timeout", + "Execution error ... timeout", + ] { + assert_eq!( + mgr.eval(&format!("(nrepl-timeout-error? \"{msg}\")")) + .unwrap(), + "true", + "{msg:?} must be detected as a timeout" + ); + } + for msg in [ + "Execution error (ExceptionInfo) at user/eval (REPL:1). boom", + "Syntax error compiling at (REPL:1:1). Unable to resolve symbol", + "nREPL connection closed by server", + ] { + assert_eq!( + mgr.eval(&format!("(nrepl-timeout-error? \"{msg}\")")) + .unwrap(), + "false", + "{msg:?} is not a timeout and must not drop the connection" + ); + } +} From 8ebad0d807fdd286dc4c5167b358ecc8915f77bb Mon Sep 17 00:00:00 2001 From: Steffen Dienst Date: Thu, 29 Oct 2026 14:40:11 +0100 Subject: [PATCH 2/2] fix(ui): accept Backspace and Ctrl+G in modal free-text fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three modal free-text fields — the `question` tool's custom answer, the plan-approval feedback box, and the permission deny-note — each hand-rolled a `match key.code` in `dispatch_modal!` that knew only `KeyCode::Backspace`. Two keys the compose box has always handled were therefore dead in every one of them: - Backspace. A terminal whose `kbs` is `^H` (MobaXterm, screen, some serial consoles) sends the raw BS byte `0x08`, which crossterm reports as Ctrl+H rather than `KeyCode::Backspace`. The compose editor binds `delete_char_back` to Ctrl+H, so it was fine; the modal matchers dropped the key entirely, and a typo could not be corrected. - Ctrl+G. The compose box opens `$EDITOR` via `external_editor`; the modals had no arm for it, so the chord did nothing. Route all three fields through one pure decider, `entry_edit(key, mods) -> EntryEdit` (Insert / Backspace / ExternalEditor / Ignore). It accepts BOTH decodings of Backspace, maps Ctrl+G to `ExternalEditor`, keeps Ctrl+C/D as `Ignore` so each caller still reads them as the panic gesture, and folds AltGr (Ctrl+Alt+printable on Windows) back to text. Ctrl+G suspends the TUI around `$EDITOR` via the new `edit_entry_in_editor()` helper, mirroring the compose path (`suspend_tui_for_subprocess` → `external_editor::edit_text` → `resume_tui_after_subprocess`); this is why `dispatch_modal!` now takes the `user_tx` channel as a second argument. One decider instead of three near-copies is also why the question field had once pushed the literal `c` of a Ctrl+C into the user's answer (dirge-x2zf): the copies drifted. A guard test scans every `.push(c)` text site in mod.rs and fails if one bypasses `EntryEdit::Insert`. Regression tests (`cargo test --bin dirge entry_`): Ctrl+H deletes like Backspace, Ctrl+G maps to the external editor, plain `g` inserts, AltGr inserts, Ctrl+C/D are inert, Enter/Esc stay with the caller. --- src/ui/mod.rs | 279 ++++++++++++++++++++++++++++---------------- src/ui/mod_tests.rs | 117 ++++++++++++++++--- 2 files changed, 282 insertions(+), 114 deletions(-) diff --git a/src/ui/mod.rs b/src/ui/mod.rs index 69893c1d1..2a5518c11 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -757,7 +757,7 @@ pub async fn run_interactive( // can borrow the same locals (`agent`, channels, `context`, …) the // former arms did. macro_rules! dispatch_modal { - ($ev:expr) => { + ($ev:expr, $user_tx:expr) => { // dirge-7543: a paste while a modal owns the input must NOT fall // through to the compose editor below. The Question custom-answer // field is the only modal that takes free text, so deliver the @@ -885,50 +885,54 @@ pub async fn run_interactive( unreachable!() }; if let Some(e) = entry { - // Typing feedback for the "edit" path. - match key.code { - KeyCode::Enter => { - let fb = e.buf.trim().to_string(); - if !fb.is_empty() { - decision = Some(PlanApprovalDecision::Edit(fb)); - } - // Empty feedback → ignore Enter, keep typing. - } - KeyCode::Esc => { - // Discard the draft, return to a/e/c. - *entry = None; - renderer.write_line( - " (edit cancelled) [a] approve [e] edit [c] cancel", - c_perm(), - )?; + // Typing feedback for the "edit" path, through the + // shared decider (Backspace accepts the BS byte; + // Ctrl+G opens $EDITOR). + match entry_edit(key.code, key.modifiers) { + EntryEdit::Insert(c) => { + e.buf.push(c); + render_custom_entry(&mut renderer, &e.buf, e.input_anchor); renderer.request_repaint(); } - KeyCode::Backspace => { + EntryEdit::Backspace => { e.buf.pop(); - render_custom_entry( - &mut renderer, - &e.buf, - e.input_anchor, - ); + render_custom_entry(&mut renderer, &e.buf, e.input_anchor); renderer.request_repaint(); } - KeyCode::Char(c) - if !key.modifiers.contains(KeyModifiers::CONTROL) => - { - e.buf.push(c); - render_custom_entry( - &mut renderer, - &e.buf, - e.input_anchor, - ); + EntryEdit::ExternalEditor => { + #[cfg(unix)] + if let Some(text) = + edit_entry_in_editor(&mut renderer, $user_tx, &e.buf) + { + e.buf = text; + } + render_custom_entry(&mut renderer, &e.buf, e.input_anchor); renderer.request_repaint(); } - // Ctrl+C bails from feedback entry too (not just - // the a/e/c screen), matching the rest of the UI. - _ if is_ctrl_c => { - decision = Some(PlanApprovalDecision::Cancel) - } - _ => {} + EntryEdit::Ignore => match key.code { + KeyCode::Enter => { + let fb = e.buf.trim().to_string(); + if !fb.is_empty() { + decision = Some(PlanApprovalDecision::Edit(fb)); + } + // Empty feedback → ignore Enter, keep typing. + } + KeyCode::Esc => { + // Discard the draft, return to a/e/c. + *entry = None; + renderer.write_line( + " (edit cancelled) [a] approve [e] edit [c] cancel", + c_perm(), + )?; + renderer.request_repaint(); + } + // Ctrl+C bails from feedback entry too (not just + // the a/e/c screen), matching the rest of the UI. + _ if is_ctrl_c => { + decision = Some(PlanApprovalDecision::Cancel) + } + _ => {} + }, } } else { // Choosing approve / edit / cancel. @@ -998,56 +1002,62 @@ pub async fn run_interactive( let num_options = question.options.len(); if let Some(entry) = &mut q.entry { - // Innermost former loop: free-form text entry. - match key.code { - KeyCode::Enter => { - q.custom_text = if entry.buf.is_empty() { - None - } else { - Some(std::mem::take(&mut entry.buf)) - }; - q.entry = None; - if !multi { - if let Some(ct) = q.custom_text.take() { - q.answers.push(vec![ct]); - } - QStep::Next - } else { - // Multi: keep going; Enter again confirms. - QStep::Stay - } - } - KeyCode::Esc => { - // Discard the typed text, back to options. - q.entry = None; + // Innermost former loop: free-form text entry, + // through the shared decider — Backspace accepts + // the BS byte, Ctrl+G opens $EDITOR. + match entry_edit(key.code, key.modifiers) { + EntryEdit::Insert(c) => { + entry.buf.push(c); QStep::Stay } - KeyCode::Backspace => { + EntryEdit::Backspace => { entry.buf.pop(); QStep::Stay } - // dirge-x2zf: unguarded, this pushed the - // literal 'c' of a Ctrl+C into the user's - // answer. `PlanApproval`'s entry has - // carried this guard all along; this one - // is the copy that never got it. - KeyCode::Char(c) - if !key.modifiers.contains(KeyModifiers::CONTROL) => - { - entry.buf.push(c); + EntryEdit::ExternalEditor => { + #[cfg(unix)] + if let Some(text) = + edit_entry_in_editor(&mut renderer, $user_tx, &entry.buf) + { + entry.buf = text; + } QStep::Stay } - // Ctrl+C bails from the answer field too, - // not just the option list. - _ if key.modifiers.contains(KeyModifiers::CONTROL) - && matches!( - key.code, - KeyCode::Char('c') | KeyCode::Char('d') - ) => - { - QStep::Rejected - } - _ => QStep::Stay, + EntryEdit::Ignore => match key.code { + KeyCode::Enter => { + q.custom_text = if entry.buf.is_empty() { + None + } else { + Some(std::mem::take(&mut entry.buf)) + }; + q.entry = None; + if !multi { + if let Some(ct) = q.custom_text.take() { + q.answers.push(vec![ct]); + } + QStep::Next + } else { + // Multi: keep going; Enter again confirms. + QStep::Stay + } + } + KeyCode::Esc => { + // Discard the typed text, back to options. + q.entry = None; + QStep::Stay + } + // Ctrl+C/D bail from the answer field + // too, not just the option list. + _ if key.modifiers.contains(KeyModifiers::CONTROL) + && matches!( + key.code, + KeyCode::Char('c') | KeyCode::Char('d') + ) => + { + QStep::Rejected + } + _ => QStep::Stay, + }, } } else { // Option-select. Decision is pure (dirge-72h2); @@ -1356,27 +1366,31 @@ pub async fn run_interactive( decision = Some(UserDecision::deny()); still_typing = false; } else { - match key.code { - KeyCode::Enter => { - let note = note.trim().to_string(); - decision = Some(UserDecision::Deny { - note: (!note.is_empty()).then_some(note), - }); - still_typing = false; - } - // Back to the action keys, draft discarded. - KeyCode::Esc => still_typing = false, - KeyCode::Backspace => { + match entry_edit(key.code, key.modifiers) { + EntryEdit::Insert(c) => note.push(c), + EntryEdit::Backspace => { note.pop(); } - KeyCode::Char(c) - if !key - .modifiers - .contains(KeyModifiers::CONTROL) => - { - note.push(c) + EntryEdit::ExternalEditor => { + #[cfg(unix)] + if let Some(text) = + edit_entry_in_editor(&mut renderer, $user_tx, ¬e) + { + note = text; + } } - _ => {} + EntryEdit::Ignore => match key.code { + KeyCode::Enter => { + let note = note.trim().to_string(); + decision = Some(UserDecision::Deny { + note: (!note.is_empty()).then_some(note), + }); + still_typing = false; + } + // Back to the action keys, draft discarded. + KeyCode::Esc => still_typing = false, + _ => {} + }, } } if still_typing { @@ -1877,7 +1891,7 @@ pub async fn run_interactive( // #387 follow-up: if a modal owns the input, route the event // there (swallowing keys) instead of the compose editor. if ui.input_mode.is_modal() { - dispatch_modal!(ev); + dispatch_modal!(ev, &user_tx); } match ev { // Mouse Down/Drag/Up that selection::handle declined @@ -5557,6 +5571,71 @@ fn option_select_action( } } +/// The effect of one keystroke on a modal free-text field (the questionnaire +/// custom answer, plan-approval feedback, permission deny note). Pure +/// (unit-tested); the caller maps it onto the field's buffer and re-renders. +/// Enter/Esc/Ctrl+C/D are NOT edits — they stay in the caller so each field +/// keeps its own submit/discard semantics. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum EntryEdit { + /// Insert this character at the end of the buffer. + Insert(char), + /// Delete the character before the caret. + Backspace, + /// Ctrl+G: open the field in `$EDITOR`. + ExternalEditor, + /// Nothing for this field to do — the caller decides (Enter submits, Esc + /// discards, Ctrl+C/D reject). + Ignore, +} + +/// Map a keystroke to an [`EntryEdit`] for a single-line free-text field. +/// +/// Crucially this accepts BOTH decodings of the Backspace key. A terminal whose +/// `kbs` is `^H` (MobaXterm, screen, several serial consoles) sends the raw BS +/// byte `0x08`, which crossterm reports as Ctrl+H rather than +/// `KeyCode::Backspace` — so a hand-rolled `KeyCode::Backspace` matcher silently +/// drops the user's Backspace. The compose editor has always handled Ctrl+H via +/// its keymap (`delete_char_back`); routing every modal field through this one +/// decider gives them the same behavior. +/// +/// `Ctrl+G` opens the external editor, matching the compose box. Ctrl+C/D stay +/// [`EntryEdit::Ignore`] so the caller can read them as the panic gesture. +fn entry_edit(key: KeyCode, mods: KeyModifiers) -> EntryEdit { + let ctrl = mods.contains(KeyModifiers::CONTROL); + let alt = mods.contains(KeyModifiers::ALT); + // AltGr on Windows arrives as Ctrl+Alt+printable — real text, not a chord + // (mirrors `input::normalize_altgr`). + let altgr = ctrl && alt; + if ctrl && !altgr && matches!(key, KeyCode::Char('c') | KeyCode::Char('d')) { + return EntryEdit::Ignore; + } + match key { + KeyCode::Char('g') if ctrl && !altgr => EntryEdit::ExternalEditor, + KeyCode::Backspace => EntryEdit::Backspace, + KeyCode::Char('h') if ctrl && !altgr => EntryEdit::Backspace, + KeyCode::Char(c) if !ctrl || altgr => EntryEdit::Insert(c), + _ => EntryEdit::Ignore, + } +} + +/// Hand a modal free-text field to `$EDITOR`, suspending the TUI around the +/// child exactly as the compose box's Ctrl+G does. Returns the edited text — the +/// field is single-line, so newlines the editor introduced collapse to spaces — +/// or `None` when the editor was aborted or there is no `/dev/tty` to suspend +/// against (in which case the field is left untouched). +#[cfg(unix)] +fn edit_entry_in_editor( + renderer: &mut Renderer, + user_tx: &tokio::sync::mpsc::UnboundedSender, + seed: &str, +) -> Option { + let _drained = terminal::suspend_tui_for_subprocess(user_tx)?; + let edited = crate::ui::external_editor::edit_text(seed, "answer"); + terminal::resume_tui_after_subprocess(renderer, user_tx); + edited.map(|s| s.replace(['\n', '\r', '\t'], " ").trim_end().to_string()) +} + /// #387 follow-up: write a question's header + soft-wrapped stem and /// return the buffer index where its option block begins (the `anchor` /// the dispatcher `replace_from`s on every keystroke). Extracted from the diff --git a/src/ui/mod_tests.rs b/src/ui/mod_tests.rs index 774a0f0c2..388825381 100644 --- a/src/ui/mod_tests.rs +++ b/src/ui/mod_tests.rs @@ -1284,18 +1284,19 @@ fn opt_esc_rejects() { ); } -/// Every modal text field must ignore control chords rather than typing -/// them (dirge-x2zf). Three fields take free text — plan-approval feedback, -/// the question tool's custom answer, and the permission deny note — and -/// two of them had carried the guard from the start. The third pushed the -/// literal `c` of a Ctrl+C into the user's answer for as long as it had -/// existed, which is how a key that cancels everywhere else came to corrupt -/// text here. +/// Every modal free-text field must route keystrokes through the shared +/// [`entry_edit`] decider rather than a hand-rolled `match key.code`. Three +/// fields take free text — plan-approval feedback, the question tool's custom +/// answer, and the permission deny note. The hand-rolled copies are how the +/// question answer lost Backspace on terminals that send the BS byte and once +/// pushed the literal `c` of a Ctrl+C into the user's answer; one decider means +/// one place to keep correct. /// -/// A count, not a parse: crude enough to fool, strict enough to catch the -/// mistake anyone actually makes — adding a fourth field and forgetting. +/// A scan, not a parse: crude enough to fool, strict enough to catch the +/// mistake anyone actually makes — adding a fourth field and hand-rolling its +/// own key handling. #[test] -fn every_modal_text_field_ignores_control_chords() { +fn every_modal_text_field_routes_through_entry_edit() { let src = include_str!("mod.rs"); let lines: Vec<&str> = src.lines().collect(); let mut fields = 0; @@ -1304,12 +1305,13 @@ fn every_modal_text_field_ignores_control_chords() { continue; } fields += 1; - // The guard rides on the `KeyCode::Char(c)` arm just above. + // The routing rides on the `EntryEdit::Insert(c)` arm (on the line + // itself, or just above for the multi-arm fields). let window = lines[i.saturating_sub(6)..=i].join("\n"); assert!( - window.contains("KeyModifiers::CONTROL"), - "the text field at mod.rs:{} types control chords instead of \ - ignoring them; guard `KeyCode::Char(c)` on `!CONTROL` as the \ + window.contains("EntryEdit::Insert"), + "the text field at mod.rs:{} pushes characters without going \ + through `entry_edit`; route it through the shared decider as the \ other modal fields do", i + 1, ); @@ -1375,6 +1377,93 @@ fn opt_space_off_options_without_custom_is_ignored() { assert_eq!(a, OptionAction::Ignore); } +// ============================================================ +// entry_edit — free-text answer field keystrokes +// ============================================================ + +#[test] +fn entry_printable_char_inserts() { + assert_eq!( + entry_edit(KC::Char('x'), KeyModifiers::NONE), + EntryEdit::Insert('x') + ); +} + +#[test] +fn entry_space_inserts() { + // Space is text here, not a select/toggle key. + assert_eq!( + entry_edit(KC::Char(' '), KeyModifiers::NONE), + EntryEdit::Insert(' ') + ); +} + +#[test] +fn entry_backspace_deletes_before_caret() { + assert_eq!( + entry_edit(KC::Backspace, KeyModifiers::NONE), + EntryEdit::Backspace + ); +} + +#[test] +fn entry_ctrl_h_deletes_like_backspace() { + // The user's terminal can report Backspace as the BS byte (0x08), which + // crossterm decodes as Ctrl+H rather than `KeyCode::Backspace`. Before the + // fix this key was silently dropped by the custom-answer field. + assert_eq!( + entry_edit(KC::Char('h'), KeyModifiers::CONTROL), + EntryEdit::Backspace + ); +} + +#[test] +fn entry_ctrl_g_opens_external_editor() { + assert_eq!( + entry_edit(KC::Char('g'), KeyModifiers::CONTROL), + EntryEdit::ExternalEditor + ); +} + +#[test] +fn entry_plain_g_inserts() { + // A bare `g` is ordinary text in an answer field, not the editor chord. + assert_eq!( + entry_edit(KC::Char('g'), KeyModifiers::NONE), + EntryEdit::Insert('g') + ); +} + +#[test] +fn entry_altgr_char_inserts() { + // Windows reports AltGr as Ctrl+Alt+printable — real text, not a chord. + assert_eq!( + entry_edit(KC::Char('@'), KeyModifiers::CONTROL | KeyModifiers::ALT), + EntryEdit::Insert('@') + ); +} + +#[test] +fn entry_enter_and_esc_are_not_edits() { + // Enter/Esc are terminal (submit/discard) and handled by the caller. + assert_eq!(entry_edit(KC::Enter, KeyModifiers::NONE), EntryEdit::Ignore); + assert_eq!(entry_edit(KC::Esc, KeyModifiers::NONE), EntryEdit::Ignore); +} + +#[test] +fn entry_ctrl_c_is_not_an_edit() { + // Ctrl+C is the panic gesture — the caller maps it to a rejection; the + // field must not push the literal 'c'. + assert_eq!( + entry_edit(KC::Char('c'), KeyModifiers::CONTROL), + EntryEdit::Ignore + ); + assert_eq!( + entry_edit(KC::Char('d'), KeyModifiers::CONTROL), + EntryEdit::Ignore + ); +} + // ============================================================ // panel_refresh_due — ambient-panel throttle (dirge-b14h) // ============================================================