feat(lua): add list mode to maki.fn.jobstart - #649
Conversation
9ed8628 to
458794d
Compare
|
Cleaned the branch to only the intended work:
|
458794d to
aaafc6f
Compare
|
Rebased PR 649 onto the cleaned PR 617 branch so the dependency bash fix is shared instead of duplicated. Final commit list:
Fixed a missing |
b2583ff to
16e8289
Compare
| // Collect remaining args, filtering out empty strings | ||
| let args: Vec<String> = (2..=len) | ||
| .filter_map(|i| tbl.get::<String>(i).ok()) | ||
| .filter(|s| !s.is_empty()) | ||
| .collect(); |
There was a problem hiding this comment.
this silently drops empty-string args (valid, e.g. -m "") and non-string items ({"git", "log", "-n", true} becomes git log -n). nvim passes empties through and errors on non-strings, let's do the same
There was a problem hiding this comment.
still not fixed, the collect is now filter_map(...).filter(|s| !s.is_empty()), so empty strings are still dropped and non-strings still silently skipped. {"rm", "-rf", accidentally_nil, "/tmp/x"} just runs with an arg missing. also mlua coerces numbers to strings, so {123, "echo"} never hits the "must be a string" error, it spawns a program called 123. check each index as a Value and error on anything that isn't a string
| fn jobstart_list_mode_preserve_arg_quoting() { | ||
| let reg = fresh_registry(); | ||
| let host = PluginHost::new(Arc::clone(®)).unwrap(); | ||
| let src = format!( | ||
| r#"maki.api.register_tool({{ | ||
| name = "job_list", | ||
| description = "runs program directly via list mode", | ||
| schema = {MINIMAL_SCHEMA}, | ||
| audiences = {{ "main" }}, | ||
| handler = function(input, ctx) | ||
| maki.fn.jobstart({{"echo", "hello world"}}, {{ | ||
| on_exit = function(_, code) | ||
| ctx:finish("exit=" .. tostring(code)) | ||
| end | ||
| }}) | ||
| end | ||
| }})"# | ||
| ); | ||
| host.load_source("job_list", &src).unwrap(); | ||
| let out = exec_tool(®, "job_list", serde_json::json!({})).unwrap(); | ||
| assert_eq!(out, "exit=0"); | ||
| } | ||
|
|
||
| /// List mode with multiple args works correctly. | ||
| #[test] | ||
| fn jobstart_list_mode_multiple_args() { | ||
| let reg = fresh_registry(); | ||
| let host = PluginHost::new(Arc::clone(®)).unwrap(); | ||
| let src = format!( | ||
| r#"maki.api.register_tool({{ | ||
| name = "job_multi", | ||
| description = "tests multiple args in list mode", | ||
| schema = {MINIMAL_SCHEMA}, | ||
| audiences = {{ "main" }}, | ||
| handler = function(input, ctx) | ||
| local seen = {{}} | ||
| local exit_code | ||
| maki.fn.jobstart({{"echo", "-n", "a", "b", "c"}}, {{ | ||
| on_stdout = function(_, line) seen[#seen + 1] = line end | ||
| }}) | ||
| local res = maki.fn.jobwait(1) | ||
| return table.concat(seen, ",") | ||
| end | ||
| }})"# | ||
| ); | ||
| host.load_source("job_multi", &src).unwrap(); | ||
| let out = exec_tool(®, "job_multi", serde_json::json!({})).unwrap(); | ||
| // echo -n a b c should output "a b c" without trailing newline | ||
| assert_eq!(out, "a b c"); | ||
| } | ||
|
|
||
| /// Empty table for list mode errors appropriately. | ||
| #[test] | ||
| fn jobstart_list_mode_empty_table_errors() { | ||
| let reg = fresh_registry(); | ||
| let host = PluginHost::new(Arc::clone(®)).unwrap(); | ||
| let src = format!( | ||
| r#"maki.api.register_tool({{ | ||
| name = "job_empty", | ||
| description = "empty array errors", | ||
| schema = {MINIMAL_SCHEMA}, | ||
| audiences = {{ "main" }}, | ||
| handler = function(input, ctx) | ||
| local _, err = pcall(maki.fn.jobstart, {{}}) | ||
| return tostring(err) | ||
| end | ||
| }})"# | ||
| ); | ||
| host.load_source("job_empty", &src).unwrap(); | ||
| let out = exec_tool(®, "job_empty", serde_json::json!({})).unwrap(); | ||
| assert!(out.contains("must have at least a program"), "got: {out}"); | ||
| } | ||
|
|
||
| /// Non-string in array errors appropriately. | ||
| #[test] | ||
| fn jobstart_list_mode_non_string_arg_errors() { | ||
| let reg = fresh_registry(); | ||
| let host = PluginHost::new(Arc::clone(®)).unwrap(); | ||
| let src = format!( | ||
| r#"maki.api.register_tool({{ | ||
| name = "job_nonstr", | ||
| description = "non-string arg errors", | ||
| schema = {MINIMAL_SCHEMA}, | ||
| audiences = {{ "main" }}, | ||
| handler = function(input, ctx) | ||
| local _, err = pcall(maki.fn.jobstart, {{123, "echo"}}) | ||
| return tostring(err) | ||
| end | ||
| }})"# | ||
| ); | ||
| host.load_source("job_nonstr", &src).unwrap(); | ||
| let out = exec_tool(®, "job_nonstr", serde_json::json!({})).unwrap(); | ||
| // When a non-string is in the array, mlua's get::<String> will error | ||
| assert!(!out.is_empty(), "got empty error string"); | ||
| } |
There was a problem hiding this comment.
these pass in shell mode too, so they can't catch a broken list mode. try asserting on something the shell would mangle, e.g. { "echo", "$HOME" } staying literal. the non-string test always passes as written (tostring(err) is never empty), assert pcall failed + the message instead
There was a problem hiding this comment.
these still pass in shell mode, the quoting test only asserts exit=0 and the multi-arg test's output is identical under bash -c. and the non-string test still asserts tostring(err) is non-empty, which is true whether it errors or not (tostring(nil) is "nil"). assert something the shell would mangle, like {"echo", "$HOME"} staying literal, and assert the actual error message
| fn jobstart(lua: &Lua, cmd: Value, opts: Option<Table>) -> LuaResult<u32> { | ||
| let spec = match cmd { | ||
| Value::String(s) => JobSpec::Shell(s.to_str()?.to_owned()), | ||
| Value::Table(tbl) => { | ||
| // Treat tables as arrays (list mode): first element is program, rest are args | ||
| let len = tbl.len().unwrap_or(0); | ||
| if len == 0 { | ||
| return Err(mlua::Error::runtime( | ||
| "jobstart array must have at least a program", | ||
| )); | ||
| } | ||
| let program: String = tbl.get::<String>(1).map_err(|e| { | ||
| mlua::Error::runtime(format!("jobstart program must be a string: {e}")) | ||
| })?; |
There was a problem hiding this comment.
nvim returns -1 on spawn failure instead of throwing, and ported plugins rely on if jobstart(...) <= 0. list mode makes ENOENT common so i'd match that. throwing on wrong types is fine
There was a problem hiding this comment.
still throws, spawn().map_err(...)? propagates to a lua error. and now the two modes disagree: a bad command in string mode gives you a job id + on_exit(127), in list mode it throws and kills the handler. docs say it returns a job id. return -1 like nvim
b3c5e82 to
3c3c763
Compare
…e command building
- find_bash_on_path: check Git/Cygwin/MSYS2 paths before PATH scan, skip %SystemRoot% entries to avoid legacy WSL bash.exe - find_wsl: verify distro installed via 'wsl.exe -e true' before trusting - bash_command: accept env map, append var names to WSLENV for WSL fallback so env vars cross the Windows/Linux boundary - shell.rs: remove redundant 'use maki_config' (clippy)
…INAL_PROMPT The WSL fallback in maki_config::bash_command builds WSLENV from the env map passed to it, but maki-ui's shell command set GIT_TERMINAL_PROMPT after the call with no way to include it in WSLENV. Pass the env map in so the variable name is forwarded across the Windows/Linux boundary, while still setting the value on the spawned process.
Allow callers to pass program arguments as a table to bypass shell interpretation and preserve argument quoting. This fixes tontinton#602. The function now accepts either: - A string: runs through bash -c (shell mode) - A table: runs program directly with args (list mode) Example: maki.fn.jobstart({ "git", "commit", "-m", "msg" }) Includes: - JobSpec enum for Shell vs Program modes - Updated JobStore::start to handle both modes - Lua binding changes to accept string|table - Tests for list mode (preserve quoting, multiple args, error cases) - Documentation updates with examples for both modes
3c3c763 to
9c1cd13
Compare
|
|
||
| **String mode** runs through `bash -c` on all platforms. On Windows, you need Git Bash or WSL installed. Use this when you need shell features like pipes, redirection, or variable expansion. | ||
|
|
||
| **List mode** runs the program directly with preserved argument quoting. Pass an array where the first element is the program and the rest are arguments. This is the safer choice for commands with spaces or special characters in arguments. |
There was a problem hiding this comment.
on a wsl-only windows box, string mode runs inside wsl but list mode spawns a native windows process, so {"ls", "-la"} fails where "ls -la" works. fine to ship, but worth a line here
| (cwd, env, on_stdout, on_stderr, on_exit) | ||
| } | ||
| None => (None, None, None, None, None), | ||
| }; |
There was a problem hiding this comment.
.unwrap_or(-1) turns every failure into -1, not just spawn failure. bad cwd and the "bash not found on windows" message with install instructions all vanish, not even logged. nvim only returns -1 for non-executable cmd[0] and throws for bad cwd, let's match that and log the dropped spawn error.
Also jobwait and jobstop still take u32, so a plugin doing jobwait(jobstart(...)) gets "error converting Lua integer to u32" on failure instead of anything useful. our own plugins/bash/init.lua does exactly that at line 112 and was never updated for the new contract. either accept i64 there and no-op on invalid ids like nvim, or this -1 is a trap.
| }; | ||
| command | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) |
There was a problem hiding this comment.
This is dead code: BufRead::lines() already strips CRLF. the only time it fires is on \r\r\n, where it eats a real \r from the data. please remove.
| assert!( | ||
| out.is_empty(), | ||
| "offset beyond file should return empty, got: {out}" | ||
| ); |
There was a problem hiding this comment.
This test is vacuous: printf %s "" outputs zero bytes, so seen is empty and the assert passes whether the empty arg is passed through or dropped. try {"sh", "-c", "echo $#", "sh", ""} and assert "1", that actually fails if the arg is filtered.
| assert!( | ||
| out.is_empty(), | ||
| "offset beyond file should return empty, got: {out}" | ||
| ); |
There was a problem hiding this comment.
{123, "echo"} hits the program branch ("jobstart program must be a string"), so the args loop is still untested. use {"echo", 123} and assert "arg 2 must be a string" so the two paths can't be confused.
| ) -> Result<u32, String> { | ||
| let mut command = shell_command(cmd); | ||
| let mut command = match spec { | ||
| JobSpec::Shell(cmd) => maki_config::bash_command(&cmd, env.as_ref())?, |
There was a problem hiding this comment.
This branch still carries the old 2afaa81 copy of 617, but 617 has moved on since review: bash_command now takes env (for WSLENV on wsl), wsl gets verified with -e true, etc. the JobSpec::Shell branch here calls the one-arg bash_command, so once 617 lands this conflicts in JobStore::start and either breaks the build or reverts those fixes. please rebase onto 617's current head so the diff here is only the list mode work.
54f00ba to
df0a069
Compare
Summary
Adds list mode to
maki.fn.jobstart(), matching Neovim'sjobstart()API pattern. This gives plugins a way to spawn processes with preserved argument quoting, avoiding shell parsing issues.Background
Issue #602 identified that the
bashtool mangles quoted arguments because it runs throughcmd.exe /Con Windows. The related PR #617 fixes the immediate Windows issue by resolving a realbash.exe, but the underlying Lua API only accepted shell strings.This PR adds a list mode to
jobstart()as the cleaner long-term primitive: plugins can pass[program, arg1, arg2, ...]directly, bypassing the shell entirely.Changes
JobStore::startnow accepts aJobSpecenum (ShellorProgram)maki.fn.jobstart()Lua API accepts:bash -c, etc.)Example
Signed-off-by: w0wl0lxd w0wl0lxd@tuta.com