diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0de3bf36b..4d33c418d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -320,7 +320,7 @@ jobs: # so the main "Run tests" step compiles them out entirely. - name: Run terminal tests run: | - cargo test -p bashkit --features terminal --lib -- terminal:: builtins::vi:: builtins::pager:: + cargo test -p bashkit --features terminal,git --lib -- terminal:: builtins::vi:: builtins::pager:: cargo test -p bashkit --features terminal --test integration -- terminal_tests l_term_ cargo test -p bashkit --features terminal --doc terminal diff --git a/crates/bashkit/src/builtins/git/cmd.rs b/crates/bashkit/src/builtins/git/cmd.rs index 091ef596b..d59dbaa90 100644 --- a/crates/bashkit/src/builtins/git/cmd.rs +++ b/crates/bashkit/src/builtins/git/cmd.rs @@ -321,11 +321,18 @@ async fn git_commit( i += 1; } - let Some(message) = message else { - return Ok(ExecResult::err( - "error: switch 'm' requires a value\n".to_string(), - 128, - )); + let message = match message { + Some(m) => m, + None => match editor_message(&ctx).await? { + Some(Ok(m)) => m, + Some(Err(result)) => return Ok(result), + None => { + return Ok(ExecResult::err( + "error: switch 'm' requires a value\n".to_string(), + 128, + )); + } + }, }; match git_client.commit(&ctx.fs, ctx.cwd, &message).await { @@ -334,6 +341,67 @@ async fn git_commit( } } +/// Commit message template shown in the editor, like git's. +#[cfg(all(feature = "git", feature = "terminal"))] +const COMMIT_TEMPLATE: &str = "\n# Please enter the commit message for your changes. Lines starting\n\ +# with '#' will be ignored, and an empty message aborts the commit.\n"; + +/// `git commit` without `-m` inside a terminal session: open `$EDITOR` on +/// `.git/COMMIT_EDITMSG`. `None` outside a terminal; `Some(Err)` is the +/// result to return (abort or editor failure). +#[cfg(all(feature = "git", feature = "terminal"))] +async fn editor_message( + ctx: &Context<'_>, +) -> Result>> { + if ctx.execution_extension::().is_none() { + return Ok(None); + } + let git_dir = crate::fs::vfs_join(ctx.cwd, ".git"); + if !ctx.fs.exists(&git_dir).await? { + return Ok(Some(Err(ExecResult::err( + format!("fatal: not a git repository: {}\n", ctx.cwd.display()), + 128, + )))); + } + let path = crate::fs::vfs_join(&git_dir, "COMMIT_EDITMSG"); + ctx.fs.write_file(&path, COMMIT_TEMPLATE.as_bytes()).await?; + match crate::builtins::edit_file(ctx, &path).await? { + None => Ok(None), + Some(Err(msg)) => Ok(Some(Err(ExecResult::err( + format!("error: {msg}\nPlease supply the message using the -m option.\n"), + 1, + )))), + Some(Ok(code)) if code != 0 => Ok(Some(Err(ExecResult::err( + "error: There was a problem with the editor 'vi'.\nPlease supply the message using the -m option.\n".to_string(), + 1, + )))), + Some(Ok(_)) => { + let raw = ctx.fs.read_file(&path).await?; + let text = String::from_utf8_lossy(&raw); + let message = text + .lines() + .filter(|l| !l.starts_with('#')) + .collect::>() + .join("\n"); + let message = message.trim(); + if message.is_empty() { + return Ok(Some(Err(ExecResult::err( + "Aborting commit due to empty commit message.\n".to_string(), + 1, + )))); + } + Ok(Some(Ok(message.to_string()))) + } + } +} + +#[cfg(all(feature = "git", not(feature = "terminal")))] +async fn editor_message( + _ctx: &Context<'_>, +) -> Result>> { + Ok(None) +} + #[cfg(feature = "git")] async fn git_status(ctx: Context<'_>, git_client: &super::GitClient) -> Result { match git_client.status(&ctx.fs, ctx.cwd).await { diff --git a/crates/bashkit/src/builtins/mod.rs b/crates/bashkit/src/builtins/mod.rs index 356eee8a2..c8ee7ed70 100644 --- a/crates/bashkit/src/builtins/mod.rs +++ b/crates/bashkit/src/builtins/mod.rs @@ -251,6 +251,8 @@ pub use vars::{Eval, Local, Readonly, Set, Shift, Shopt, Times, Unset}; pub use verify::Verify; #[cfg(feature = "terminal")] pub use vi::Vi; +#[cfg(all(feature = "terminal", feature = "git"))] +pub(crate) use vi::edit_file; pub use wait::Wait; pub use wc::Wc; pub use yes::Yes; diff --git a/crates/bashkit/src/builtins/vi.rs b/crates/bashkit/src/builtins/vi.rs index 636f5dea3..25b7d2753 100644 --- a/crates/bashkit/src/builtins/vi.rs +++ b/crates/bashkit/src/builtins/vi.rs @@ -16,8 +16,13 @@ //! it are dropped, and the undo history is capped by total bytes. //! - Writes go through the session VFS, so VFS limits and read-only mounts //! apply to `:w` like any other write. +//! - `:cq` quits with exit status 1, so callers that open `$EDITOR` +//! (`git commit`) can abort like with real vi. +//! - [`edit_file`] is the `$EDITOR` entry point for other builtins. Only +//! `vi`/`vim` (any directory, any args after the name) are accepted as +//! `$VISUAL`/`$EDITOR`; anything else is reported, not run. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use async_trait::async_trait; @@ -65,10 +70,52 @@ impl Builtin for Vi { )); }; let tty = tty.try_with(Clone::clone).map_err(|_| Error::Cancelled)?; - let path = file.map(|f| resolve_path(ctx.cwd, f)); + run_on(&tty, &ctx, path, file.cloned()).await + } +} + +/// Open `path` in the editor on the session terminal, like a program +/// running `$EDITOR path`. Returns `None` outside a terminal session, else +/// the editor's exit status (0 on `:wq`/`:q`, 1 on `:cq`), or an error +/// message when `$VISUAL`/`$EDITOR` names an editor bashkit does not have. +#[cfg_attr(not(feature = "git"), allow(dead_code))] +pub(crate) async fn edit_file( + ctx: &Context<'_>, + path: &Path, +) -> Result>> { + let Some(tty) = ctx.execution_extension::() else { + return Ok(None); + }; + let tty = tty.try_with(Clone::clone).map_err(|_| Error::Cancelled)?; + let editor = ["VISUAL", "EDITOR"] + .iter() + .find_map(|k| ctx.env.get(*k).or_else(|| ctx.variables.get(*k))) + .map(|v| v.trim()) + .filter(|v| !v.is_empty()); + if let Some(editor) = editor { + let program = editor.split_whitespace().next().unwrap_or(editor); + let name = program.rsplit('/').next().unwrap_or(program); + if !matches!(name, "vi" | "vim") { + return Ok(Some(Err(format!( + "editor '{editor}' is not available (supported: vi)" + )))); + } + } + let shown = path.display().to_string(); + let result = run_on(&tty, ctx, Some(path.to_path_buf()), Some(shown)).await?; + Ok(Some(Ok(result.exit_code))) +} + +async fn run_on( + tty: &Tty, + ctx: &Context<'_>, + path: Option, + display_name: Option, +) -> Result { + { let mut editor = Editor::new(tty.size().rows, tty.size().cols); - editor.display_name = file.cloned(); + editor.display_name = display_name; if let Some(path) = &path { match ctx.fs.stat(path).await { Ok(meta) if meta.file_type.is_dir() => { @@ -99,9 +146,11 @@ impl Builtin for Vi { editor.path = path; editor.saved_hash = editor.buffer_hash(); - let _screen = ScreenGuard::enter(&tty, true); - editor.run(&tty, &ctx).await; - Ok(ExecResult::ok("")) + let _screen = ScreenGuard::enter(tty, true); + editor.run(tty, ctx).await; + let mut result = ExecResult::ok(""); + result.exit_code = editor.exit_code; + Ok(result) } } @@ -156,6 +205,8 @@ struct Editor { /// Ex command queued by a normal-mode key (`ZZ`), run by the async loop. pending_ex: Option, quit: bool, + /// Exit status: 1 after `:cq`. + exit_code: i32, } impl Editor { @@ -182,6 +233,7 @@ impl Editor { last_search: None, pending_ex: None, quit: false, + exit_code: 0, } } @@ -1014,7 +1066,11 @@ impl Editor { self.quit = true; } } - "q!" | "quit!" | "cq" => self.quit = true, + "q!" | "quit!" => self.quit = true, + "cq" | "cq!" | "cquit" => { + self.exit_code = 1; + self.quit = true; + } "w" | "w!" | "write" => { self.write(arg, ctx).await; } diff --git a/crates/bashkit/src/terminal/mod.rs b/crates/bashkit/src/terminal/mod.rs index fabd08ae5..47716a9d5 100644 --- a/crates/bashkit/src/terminal/mod.rs +++ b/crates/bashkit/src/terminal/mod.rs @@ -425,6 +425,11 @@ async fn shell_loop(mut bash: crate::Bash, tty: Tty, exit: Arc, log: if !bash.shell_state_view().env.contains_key("TERM") { bash.set_env("TERM", "xterm-256color"); } + // Programs that open an editor (`git commit`) read $EDITOR; vi is the + // one bashkit ships. + if !bash.shell_state_view().env.contains_key("EDITOR") { + bash.set_env("EDITOR", "vi"); + } let mut history = History::default(); loop { log.set_activity(TerminalActivity::Prompt); @@ -839,6 +844,81 @@ mod tests { term.run_until_idle().await } + #[cfg(feature = "git")] + async fn git_repo_term() -> Terminal { + let mut term = Terminal::new(Bash::builder().git(crate::GitConfig::new())); + run( + &mut term, + "mkdir /r && cd /r && git init -q && echo x > f && git add f\r", + ) + .await; + term + } + + #[cfg(feature = "git")] + #[tokio::test] + async fn git_commit_without_m_opens_editor() { + let mut term = git_repo_term().await; + run(&mut term, "git commit\r").await; + assert!(term.is_alternate_screen()); + assert!( + term.screen_text() + .contains("# Please enter the commit message") + ); + run(&mut term, "Ofirst line\x1b:wq\r").await; + assert!(!term.is_alternate_screen(), "{}", term.screen_text()); + run(&mut term, "git log --oneline\r").await; + assert!( + term.screen_text().contains(" first line"), + "{}", + term.screen_text() + ); + } + + #[cfg(feature = "git")] + #[tokio::test] + async fn git_commit_editor_abort_paths() { + let mut term = git_repo_term().await; + // Template only: empty message aborts. + run(&mut term, "git commit; echo st=$?\r").await; + run(&mut term, ":wq\r").await; + let text = term.screen_text(); + assert!( + text.contains("Aborting commit due to empty commit message."), + "{text}" + ); + assert!(text.contains("st=1"), "{text}"); + // :cq aborts even with text typed. + run(&mut term, "git commit; echo st=$?\r").await; + run(&mut term, "Omsg\x1b:cq\r").await; + assert!(term.screen_text().contains("problem with the editor")); + // An editor bashkit lacks is reported, not run. + run(&mut term, "EDITOR=emacs git commit; echo st=$?\r").await; + assert!(!term.is_alternate_screen()); + assert!( + term.screen_text() + .contains("editor 'emacs' is not available") + ); + run(&mut term, "echo $EDITOR\r").await; + assert!( + term.screen_text().ends_with("vi\n$"), + "{}", + term.screen_text() + ); + } + + #[tokio::test] + async fn vi_cq_exits_nonzero() { + let mut term = Terminal::new(Bash::builder()); + run(&mut term, "vi /tmp/a; echo st=$?\r").await; + run(&mut term, ":cq\r").await; + assert!( + term.screen_text().contains("st=1"), + "{}", + term.screen_text() + ); + } + #[tokio::test] async fn prompt_and_command_output_render_on_screen() { let mut term = Terminal::new(Bash::builder()); diff --git a/docs/terminal.md b/docs/terminal.md index 42950e05b..5a0a221c7 100644 --- a/docs/terminal.md +++ b/docs/terminal.md @@ -147,8 +147,10 @@ the `PS2` prompt and waits for more lines. Shell state persists between lines, as in any `Bash` session. `PS1` and `PS2` are honoured (`\u \h \w \W \$`); the default prompt is `$ `. -`[ -t 0 ]` is true inside the session, and `COLUMNS`, `LINES` and `TERM` are -set. `resize()` changes the size; a running `vi` redraws. +`[ -t 0 ]` is true inside the session, and `COLUMNS`, `LINES`, `TERM` and +`EDITOR` (`vi`) are set. `git commit` without `-m` opens the editor on the +commit message, as in real git: save and quit to commit, `:cq` or an empty +message aborts. Only `vi`/`vim` work as `$EDITOR`. `resize()` changes the size; a running `vi` redraws. ## As an LLM tool @@ -267,7 +269,8 @@ shell screen. It supports: `yy yw Y`, `p P`, `r`, `J`, `u`, Ctrl-R. - **Search and replace:** `/pattern`, `n N`, `:s/pat/rep/`, `:%s/pat/rep/g` (regex patterns; `&` in the replacement is the match). -- **Files:** `:w`, `:w FILE`, `:q`, `:q!`, `:wq`, `:x`, `ZZ`, `ZQ`. +- **Files:** `:w`, `:w FILE`, `:q`, `:q!`, `:wq`, `:x`, `ZZ`, `ZQ`, `:cq` + (quit with exit status 1). `:q` refuses to discard unsaved changes, as in vim. Not supported: visual mode, named registers, macros, splits, vimrc, and `:!` diff --git a/knowledge/integrations/git-support.md b/knowledge/integrations/git-support.md index 3dd0e7ebe..c6e3647b1 100644 --- a/knowledge/integrations/git-support.md +++ b/knowledge/integrations/git-support.md @@ -21,7 +21,8 @@ on the virtual filesystem only. Configure via ### Supported Commands -- Phase 1 (local): `init`, `config`, `add`, `commit -m`, `status`, `log [-n N]` +- Phase 1 (local): `init`, `config`, `add`, `commit -m` (no `-m` opens `$EDITOR` inside a + terminal session, see [In-Process Terminal](in-process-terminal.md)), `status`, `log [-n N]` - Phase 2 (remote, virtual mode): `remote [-v]`, `remote add/remove` (fully functional); `clone`/`push`/`pull`/`fetch` validate URL against allowlist then return virtual-mode messages (no network in VFS-only mode) diff --git a/knowledge/integrations/in-process-terminal.md b/knowledge/integrations/in-process-terminal.md index bce52c15e..089388990 100644 --- a/knowledge/integrations/in-process-terminal.md +++ b/knowledge/integrations/in-process-terminal.md @@ -156,7 +156,21 @@ load/write, so entering insert mode and leaving without changes is not dirty. Bounds (TM-DOS-119): 8 MiB buffer (load/insert/put/substitute), 16 MiB undo history, 1 MiB compiled regex. Writes go through the session VFS. -Without a terminal, `vi` exits 1 (`vi: not a terminal`). +Without a terminal, `vi` exits 1 (`vi: not a terminal`). `:cq` quits with +status 1. + +## Decision: $EDITOR flows run vi in-process + +Builtins that need an editor call `builtins::edit_file(ctx, path)`, which runs +the vi editor on the session terminal from inside the calling builtin (no +re-entry into the shell, no subprocess). It returns `None` outside a terminal +so callers keep their non-interactive behavior. `$VISUAL` then `$EDITOR` are +honored only to the extent of naming `vi`/`vim` (any path, args ignored); +any other editor is reported as unavailable instead of silently swapping in +vi. A session sets `EDITOR=vi` when unset. Consumers: `git commit` without +`-m` (edits `.git/COMMIT_EDITMSG` with git's template, `#` lines dropped, +empty message or `:cq` aborts with exit 1). `git rebase -i` and `crontab -e` +are not consumers because neither command exists in bashkit. ## less / more diff --git a/knowledge/operations/limitations.md b/knowledge/operations/limitations.md index b021a0deb..fc0171aec 100644 --- a/knowledge/operations/limitations.md +++ b/knowledge/operations/limitations.md @@ -197,7 +197,7 @@ PTY running GNU bash and vim. | ID | Limitation | Why | Evidence | |----|------------|-----|----------| -| L-TERM-001 | `vi` is a subset editor: normal/insert/command-line modes, counts, common motions and operators, undo/redo, `/` search, `:s`, `:w :q :wq :x`. No visual mode, named registers, macros, splits, vimrc, or `:!` shell escape; unsupported keys are ignored | Enough to edit config and source files from an agent or a browser terminal without vendoring an editor; `:!` would re-enter the shell from inside a running command | `l_term_001_vi_is_a_subset` | +| L-TERM-001 | `vi` is a subset editor: normal/insert/command-line modes, counts, common motions and operators, undo/redo, `/` search, `:s`, `:w :q :wq :x :cq`. No visual mode, named registers, macros, splits, vimrc, or `:!` shell escape; unsupported keys are ignored | Enough to edit config and source files from an agent or a browser terminal without vendoring an editor; `:!` would re-enter the shell from inside a running command | `l_term_001_vi_is_a_subset` | | L-TERM-002 | Command stdin is not wired to the terminal: `read` and `cat` with no file get EOF instead of waiting for typed input. Only programs that read the terminal device directly (`vi`) see keystrokes | Same root as L-CLI-002, the interpreter takes stdin as a value before the command starts | `l_term_002_read_does_not_wait_for_terminal_input` | | L-TERM-003 | **Removed:** Ctrl-C now drops the running command mid-way (a running `sleep 5` stops at once) and `$?` is 130 | The exec future races an interrupt `Notify` and reuses the timeout path's `clear_cancelled_execution_state` cleanup | `terminal::tests::ctrl_c_interrupts_a_running_command` | | L-TERM-004 | Inside a terminal session, `less` and `more` page even when stdout is redirected (`less file > out` shows the pager and writes nothing to `out`). Outside a session they are always cat-like | Builtins cannot see whether their stdout is a pipe or redirect; TTY state is per-session, not per-fd-redirect | `builtins::pager::tests::pagers_are_cat_like_without_terminal` |