From 7d62eb5659dee3dd10e907c7a4c8a318c4a72169 Mon Sep 17 00:00:00 2001 From: Markus Fritsche Date: Sun, 10 May 2026 20:05:23 +0000 Subject: [PATCH] review followups: pcall shield, :resume guard, shell quoting, nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CONCERNs from the Phase 1 review pass: ffi/curl.lua: - SSE write_cb body is now pcall-wrapped. A Lua error in on_event (or in the parse loop itself) is captured into cb_error and surfaced after curl_easy_perform rather than propagating across the FFI callback boundary (which LuaJIT documents as process-fatal). The EOS flush path gets the same shield. Errors return (nil, "callback: ") from post_sse. history.lua: - sh_singlequote() escapes shell metacharacters; the mkdir -p and ls -1 shell-outs no longer double-quote (where $(...) and $VAR still expand) — single-quote with embedded-' escaping is the safe form. - M.load now returns (turns, meta) instead of (meta, turns). turns is ALWAYS a table on success, never nil-when-no-header; failure path is the unambiguous (nil, err). Callers can `if not turns then` without the previous ambiguity. repl.lua :resume updated to the new shape. repl.lua :resume: - Refuse to resume into a non-empty ctx — silent overwrite was the Q15 default, but the review surfaced the no-undo / no-warning failure mode. User must :reset (or :save then re-launch) to express intent. The current session's on-disk log is unaffected either way. NITs: - ffi/libc.lua READ_BUF: comment noting it's module-shared and Phase 1 has no reentrant readers; revisit when that changes. - PHASE1.md §7: \C-x\C-c reservation pinned to Phase 3 ("deferred from Phase 1 — no consumer here") rather than the previous dangling "(or here)". Regression suite verifies: - history.load new signature on success + failure paths - shell-quoted history.dir with $ doesn't trip - aish scripted run: ctx with 2 turns refuses :resume anchor with a clear status; user must :reset first Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/PHASE1.md | 2 +- ffi/curl.lua | 74 +++++++++++++++++++++++++++----------------------- ffi/libc.lua | 3 ++ history.lua | 25 +++++++++++------ repl.lua | 16 ++++++++--- 5 files changed, 73 insertions(+), 47 deletions(-) diff --git a/docs/PHASE1.md b/docs/PHASE1.md index 3958d21..0bc4e2d 100644 --- a/docs/PHASE1.md +++ b/docs/PHASE1.md @@ -217,7 +217,7 @@ here so subsequent phases don't collide: | Sequence | Reserved for | Phase | |---|---|---| | `\C-n` | Norris autonomous mode toggle | 3 | -| `\C-x\C-c` | Cancel running CMD: confirm prompt | 3 (or here) | +| `\C-x\C-c` | Cancel running CMD: confirm prompt | 3 (deferred from Phase 1 — no consumer here) | Phase 1 binds `\C-n` to a no-op handler that emits a `[aish] Norris mode not yet implemented (Phase 3)` status, just to verify the wiring works. diff --git a/ffi/curl.lua b/ffi/curl.lua index 682daf8..2143bd4 100644 --- a/ffi/curl.lua +++ b/ffi/curl.lua @@ -133,38 +133,40 @@ function M.post_sse(url, body, headers, on_event, timeout_ms) -- SSE parse state: buffer holds incomplete tail between callback deliveries. local buffer = "" + local cb_error = nil local write_cb = ffi.cast( "size_t(*)(char*, size_t, size_t, void*)", function(ptr, size, nmemb, _) local n = tonumber(size) * tonumber(nmemb) - buffer = buffer .. ffi.string(ptr, n) + -- pcall-wrap so a Lua error in on_event (or in the parse loop) + -- doesn't propagate across the FFI callback boundary — LuaJIT + -- documents that as process-fatal. Surface via cb_error and let + -- curl keep draining (return n) so we can report after perform. + local ok, err = pcall(function() + buffer = buffer .. ffi.string(ptr, n) + while true do + local b = buffer:find("\n\n", 1, true) + if not b then break end + local event = buffer:sub(1, b - 1) + buffer = buffer:sub(b + 2) - -- Drain complete events (terminated by \n\n). - while true do - local b = buffer:find("\n\n", 1, true) - if not b then break end - local event = buffer:sub(1, b - 1) - buffer = buffer:sub(b + 2) - - -- A single event can have multiple field lines; we only need - -- `data: ...` (joining multi-line data per the SSE spec). - local data_parts = {} - for line in (event .. "\n"):gmatch("([^\n]*)\n") do - if line:sub(1, 1) == ":" then - -- SSE keepalive comment; ignore. - elseif line:sub(1, 6) == "data: " then - data_parts[#data_parts + 1] = line:sub(7) - elseif line:sub(1, 5) == "data:" then - -- spec allows `data:` with no space - data_parts[#data_parts + 1] = line:sub(6) + local data_parts = {} + for line in (event .. "\n"):gmatch("([^\n]*)\n") do + if line:sub(1, 1) == ":" then + -- SSE keepalive comment; ignore. + elseif line:sub(1, 6) == "data: " then + data_parts[#data_parts + 1] = line:sub(7) + elseif line:sub(1, 5) == "data:" then + data_parts[#data_parts + 1] = line:sub(6) + end + end + if #data_parts > 0 then + on_event(table.concat(data_parts, "\n")) end end - if #data_parts > 0 then - on_event(table.concat(data_parts, "\n")) - end - end - + end) + if not ok and not cb_error then cb_error = err end return n end) @@ -192,24 +194,28 @@ function M.post_sse(url, body, headers, on_event, timeout_ms) -- End-of-stream flush: the final event may lack a trailing \n\n if the -- server closed the connection right after writing the last data: line -- (some llama.cpp builds, and any plain HTTP/1.0 close-on-EOF feed). - -- Parse any remaining buffer content as one last event. + -- Parse any remaining buffer content as one last event. Same pcall shield. if rc == 0 and #buffer > 0 then - local data_parts = {} - for line in (buffer .. "\n"):gmatch("([^\n]*)\n") do - if line:sub(1, 6) == "data: " then - data_parts[#data_parts + 1] = line:sub(7) - elseif line:sub(1, 5) == "data:" then - data_parts[#data_parts + 1] = line:sub(6) + local ok, perr = pcall(function() + local data_parts = {} + for line in (buffer .. "\n"):gmatch("([^\n]*)\n") do + if line:sub(1, 6) == "data: " then + data_parts[#data_parts + 1] = line:sub(7) + elseif line:sub(1, 5) == "data:" then + data_parts[#data_parts + 1] = line:sub(6) + end end - end - if #data_parts > 0 then on_event(table.concat(data_parts, "\n")) end + if #data_parts > 0 then on_event(table.concat(data_parts, "\n")) end + end) + if not ok and not cb_error then cb_error = perr end end C.curl_easy_cleanup(handle) if slist ~= nil then C.curl_slist_free_all(slist) end write_cb:free() - if rc == 0 then return true end + if cb_error then return nil, "callback: " .. tostring(cb_error) end + if rc == 0 then return true end return nil, err end diff --git a/ffi/libc.lua b/ffi/libc.lua index b22123b..24334dc 100644 --- a/ffi/libc.lua +++ b/ffi/libc.lua @@ -88,6 +88,9 @@ end -- Used by ffi/pty for master-fd transfer. Errors return nil + errmsg so -- callers can decide between EAGAIN/EINTR retry and abort. EOF on read is -- represented as ("", 0) — empty string, zero bytes. +-- Note: READ_BUF is module-shared. Phase 1 has no reentrant M.read callers +-- (no coroutines, no concurrent FFI callbacks performing reads); revisit if +-- that ever changes. local READ_BUF = ffi.new("char[?]", 4096) function M.read(fd, count) diff --git a/history.lua b/history.lua index ee777f9..e675ce5 100644 --- a/history.lua +++ b/history.lua @@ -10,10 +10,16 @@ local M = {} local Session = {} Session.__index = Session --- Best-effort mkdir -p. Failures are surfaced by io.open below. +-- Best-effort mkdir -p. Failures are surfaced by io.open below. Uses +-- single-quote escaping (Lua's %q double-quotes, which still expands $(...) +-- and $VAR inside) so a path containing shell metacharacters doesn't trip. +local function sh_singlequote(s) + return "'" .. s:gsub("'", "'\\''") .. "'" +end + local function ensure_dir(path) if not path or path == "" then return end - os.execute(string.format("mkdir -p %q", path)) + os.execute("mkdir -p " .. sh_singlequote(path)) end local function parent_dir(path) @@ -70,9 +76,11 @@ function Session:close() end -- Load a session file. Returns: --- meta : the {meta={...}} table from the first line, or nil if absent --- turns : array of {role, content, ...} for each parseable subsequent line --- nil, err : on file open failure +-- turns, meta : turns is ALWAYS a table on success (possibly empty); +-- meta is the {meta={...}} header value or nil if absent +-- nil, err : on file open failure (turns-first means callers can +-- test `if not turns then` without ambiguity vs a missing +-- meta-header line) function M.load(path) local fh, err = io.open(path, "r") if not fh then return nil, err end @@ -95,7 +103,7 @@ function M.load(path) end end fh:close() - return meta, turns + return turns, meta end -- List session files in `dir` (just file basenames matching *.jsonl). Phase 1 @@ -107,8 +115,9 @@ function M.list_sessions(dir) local out = {} if not dir or dir == "" then return out end -- io.popen here is plain ls; executor.exec was swapped to PTY but - -- io.popen itself still works. - local p = io.popen(string.format("ls -1 %q 2>/dev/null", dir)) + -- io.popen itself still works. Single-quote escaping for path safety + -- (see sh_singlequote rationale above). + local p = io.popen("ls -1 " .. sh_singlequote(dir) .. " 2>/dev/null") if not p then return out end for name in p:lines() do if name:match("%.jsonl$") then out[#out + 1] = name end diff --git a/repl.lua b/repl.lua index 7c70381..bed89bf 100644 --- a/repl.lua +++ b/repl.lua @@ -230,16 +230,24 @@ function M.run(config) local name = args:match("^%s*(%S+)") if not name then renderer.status("usage: :resume "); return end if not sessions_dir then renderer.status("(no history.dir configured)"); return end + -- Refuse to silently clobber an active conversation; the user has + -- to :reset first to express intent. The current session log on + -- disk is unaffected by either choice. + if #ctx.turns > 0 then + renderer.status(("resume into non-empty ctx refused (%d turns); :reset first") + :format(#ctx.turns)) + return + end name = name:gsub("%.jsonl$", "") local path = sessions_dir .. "/" .. name .. ".jsonl" - local meta_hdr, turns = history.load(path) - if not (meta_hdr or turns) then + local turns, _meta_hdr = history.load(path) + if not turns then renderer.status("resume failed: cannot load " .. path) return end ctx:reset() - for _, t in ipairs(turns or {}) do ctx:append(t) end - renderer.status(("resumed %d turns from %s"):format(#(turns or {}), name)) + for _, t in ipairs(turns) do ctx:append(t) end + renderer.status(("resumed %d turns from %s"):format(#turns, name)) end, help = function() io.write(HELP) end, }