marfrit/aish
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

review followups: pcall shield, :resume guard, shell quoting, nits

Browse Source 
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: <msg>") 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) <noreply@anthropic.com>
This commit is contained in:
Markus Fritsche
2026-05-10 20:05:23 +00:00
parent 1f1065157e
commit 7d62eb5659
5 changed files with 73 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/PHASE1.md
+1 -1
View File
@@ -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.
ffi/curl.lua
+40 -34
View File
@@ -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
ffi/libc.lua
+3
View File
@@ -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)
history.lua
+17 -8
View File
@@ -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
repl.lua
+12 -4
View File
@@ -230,16 +230,24 @@ function M.run(config)
local name = args:match("^%s*(%S+)")
if not name then renderer.status("usage: :resume <name>"); 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,
}