diff --git a/docs/PHASE6.md b/docs/PHASE6.md index 8c97f99..2301f46 100644 --- a/docs/PHASE6.md +++ b/docs/PHASE6.md @@ -2,9 +2,106 @@ **Project:** aish — AI-augmented conversational shell **Document:** Phase 6 Requirements, Architecture & Design Decisions -**Status:** Plan (formulate + analyze + baseline complete; tree at `9f50206`) +**Status:** Plan + review fold-in (tree at `4407029`) **Date:** 2026-05-16 +**Review findings (independent agent, 2026-05-16) — 2 BLOCKERs resolved +in-place, 7 CONCERNs folded, 6 NITs applied:** + +R1 (BLOCKER, RESOLVED). **§4 fence detector's `outside`-state branch + drops the leading `'``'` chunk of a split-fence.** The §4 + pseudocode as written ("look for ` ```\n ` in chunk; if found + [...] else: emit chunk as-is") emits the partial-fence chunk + immediately, so the next chunk no longer sees the full marker. + Contradicts B2's split-fence requirement. **Fix folded into §4:** + `outside`-state also holds a small tail (up to 10 chars) when the + chunk's tail could be a fence prefix; flushes on next push. Same + pattern as the `secrets.lua` streaming rehydrator (`secrets.lua` + ~213). Pseudocode + algorithm updated. + +R2 (BLOCKER, RESOLVED). **`highlighted()` file placement was ambiguous + in §3 vs §12.** `highlighted()` needs `_shq` (currently a `repl.lua` + M.run-local closure) and `require("executor")`. **Resolution:** + `highlighted()` stays in `repl.lua`; `renderer.lua` exposes + `renderer.set_highlight(enabled, detected, highlight_fn)`. The + filter state machine in `renderer.lua` calls back through + `highlight_fn(body, lang)` at fence-close. No `executor` dependency + in `renderer.lua`; no `_shq` lift. §3 + §12 commit 5 updated to + state this explicitly. + +R3 (CONCERN, FOLDED). **PTY raw-mode toggle per code block.** Each + `executor.exec` call calls `libc.set_raw(0)` briefly. For an + assistant turn with N fenced blocks that's N raw-mode toggles + on the streaming hot path. Smoke-test for cursor/flicker before + locking in. Added to §12 commit 5 risk row. + +R4 (CONCERN, FOLDED — risk noted, needs verify at implement-time). + **`tree-sitter highlight --lang X` invocation grammar is + unverified.** The upstream `tree-sitter` CLI's `highlight` + subcommand canonically takes a path argument and infers language + from the file extension via `~/.config/tree-sitter/config.json`. + A `--lang` flag may not exist. Since B4 confirmed zero fleet hosts + have tree-sitter installed, this can't be probed locally. + **Resolution:** §4 amended — at commit 5 implement time, VERIFY + against a real install. If `--lang` is wrong, switch to writing + the tmpfile with the matching extension (`/tmp/lua_XXX.py`) and + pass the path. Path-based discovery is the CLI's documented + primary mode. + +R5 (CONCERN, FOLDED). **`:tree off` semantics ambiguous.** §6 listed + it as "clear ctx.project" but didn't clarify whether subsequent + `:tree` (no arg) re-uses cached opts or falls back to config + defaults. Clarified in §6: `:tree off` is a one-shot clear of + `ctx.project`; subsequent `:tree` re-scans with config defaults + or the explicit arg if given. + +R6 (CONCERN, FOLDED). **cwd-coupling differs between `:diff` and + `:tree`.** `:diff` reads `libc.getcwd()` at meta invocation + time; `:tree`'s captured `ctx.project` is fixed at scan time + (per A8). After `cd /other-project`, `:diff` shows the new + project's diff but `ctx.project` still holds the old project's + tree. Documented in §5 (the diff section now cross-refs §6 / A8) + so the user-facing expectation is clear. + +R7 (CONCERN, FOLDED). **`:tree refresh` opts caching unspecified.** + Should `:tree refresh` re-use the last explicit `:tree ` depth + override, or fall back to `cfg.project.tree_depth`? Resolution: + cache the last opts on `ctx._project_opts`; `:tree refresh` reuses + them; falls back to config defaults if no prior call. §6 updated. + +R8 (CONCERN, FOLDED). **`:reset` interaction with `ctx.project`.** + Phase 4 established that `:reset` does NOT clear `ctx.memory_items` + (parity is desirable — startup-injected facts persist across a + user-driven context reset). `ctx.project` should follow the same + rule: `:reset` clears `ctx.turns` and `pending_exec_output` and + `ctx.summary` (per `Context:reset` at `context.lua` ~343), but + NOT memory_items and NOT project. Documented in §3 + §12 commit 1. + +R9 (CONCERN, FOLDED). **Status-bump duplication between §12 commits 5 + and 6.** Commit 5 sub-step (e) said "PHASE6 status → Implement"; + commit 6 also said the same. Resolved: commit 5e does NOT bump + the status (only HELP update); commit 6 owns the status bump + (along with the config example). One owner per change. + +R-N1..N6 (NITs, APPLIED): + N1. §4 algorithm pseudocode now includes the SOL/post-newline + anchor requirement (mid-line backticks in prose don't open a + fence). The plan §12 risk row already promised this; now §4 + matches. + N2. §4 detection block gained a comment explaining the `read("*l") + and pipe:close()` pattern — close return-value is ignored per + B3; presence of an output line is the signal. + N3. §5 `:diff staged → git diff --cached` table row dropped (the + meta is a thin pass-through; user types the right git flags). + `:diff --cached` works directly. Surface is honest. + N4. §6 `_scan_project_tree` switched from `os.execute("cd " .. shq + .. " && git rev-parse ...")` to `git -C rev-parse + --git-dir` — no subshell, more idiomatic. + N5. §12 "Open at plan-time" first bullet (dir-arg vs hardcoded + getcwd) dropped — already decided in §6's signature; not open. + N6. §11 wording on Phase 7+ left as-is (reviewer marked purely + cosmetic). + **Analyze findings (2026-05-16):** A1. **renderer.lua surface clean** — `assistant_delta(chunk)` already @@ -191,6 +288,9 @@ filter grows past ~80 LOC, lift it into `highlight.lua` as a follow-up. ```lua local function _detect_treesitter() local pipe = io.popen("command -v tree-sitter 2>/dev/null && tree-sitter --version 2>/dev/null") + -- N2 / B3: pipe:close() returns true on LuaJIT regardless of exit + -- code; we don't use it for the verdict. Presence of an output + -- line from --version is the actual signal. local ok = pipe and pipe:read("*l") and pipe:close() return ok end @@ -202,71 +302,128 @@ tree-sitter and re-toggle. ### Stream filter -The filter wraps `renderer.assistant_delta`. State machine: +The filter wraps `renderer.assistant_delta`. State machine (R1 + N1 +revisions — outside-state accumulator + SOL anchor): ``` -state = "outside" | "inside" -buf = "" -- only used in "inside" -lang = nil -- captured at fence open +state = "outside" | "inside" +tail = "" -- outside-state lookahead buffer (R1) +buf = "" -- only used in "inside" +lang = nil -- captured at fence open push(chunk): if state == "outside": - look for ```\n in chunk - if found: - emit chunk up to fence-open - state = "inside"; lang = parsed; buf = chunk after fence-open + combined = tail .. chunk + -- R1: hold back trailing partial-fence so a split fence + -- ("``" arrives, then "`python\n") doesn't get emitted + -- as plain text before we recognize the opener. + -- N1: fence opens only at start-of-stream OR after a newline + -- ("^```" or "\n```"). Inline backticks in prose don't open. + match_pos = find(combined, "(^|\n)```([%w_-]*)\n") + if match_pos: + -- everything before the opening is plain text + emit combined[1 .. fence_start - 1] + lang = captured_lang + buf = combined[fence_end .. end] -- text after \n + state = "inside"; tail = "" + if buf has \n``` inside, fall through to inside-state below else: - emit chunk as-is + -- Hold back the last K chars if they could be the start + -- of a fence-open. Specifically: tail = the longest suffix + -- of combined that is a prefix of any well-formed fence + -- marker ("`", "``", "```", "```l", "```lua", "```lua\n"). + -- Bounded by max-lang-tag-length + 4 (~10 chars in practice). + tail = longest_partial_fence_suffix(combined, max=10) + emit combined[1 .. #combined - #tail] + -- (next push will combine tail with the next chunk and retry) if state == "inside": buf = buf .. chunk - look for \n``` in buf - if found: - fence_body = buf up to closing - rest = buf after closing + -- closing fence: "\n```" anywhere in buf (followed by EOL or end). + close_pos = find(buf, "\n```") + if close_pos: + fence_body = buf[1 .. close_pos - 1] + closing = buf[close_pos .. close_pos + 3] -- "\n```" + rest = buf[close_pos + 4 .. end] emit highlighted(fence_body, lang) - emit closing fence verbatim - emit rest as-is (recurse with state="outside") - state = "outside"; buf = "" + emit closing verbatim + state = "outside"; buf = ""; tail = "" + if rest != "": + push(rest) -- recurse for any plain text after the closing else: -- still buffering; nothing emitted this push ``` -Edge cases: chunk boundary lands inside the fence marker itself -(e.g., chunk ends with ` `` `, next starts with `\n`). The state -machine looks at the cumulative `buf`, so partial markers are -recovered correctly. +Edge cases: +- Chunk boundary lands inside an opening marker (e.g., chunk ends with + `'``'`, next starts with `'`python\n'`). The `tail` buffer holds + `'``'`; next push combines and finds the full opener. +- Chunk boundary inside a closing marker. The `inside` branch already + accumulates into `buf`; `find` against cumulative `buf` recovers. +- Inline backticks in prose (`"use ``` to mark code"`). N1's + `(^|\n)```` anchor means this does NOT open a fence — `\n` is + required before the three backticks. -`highlighted(body, lang)` — **B3-revised** (supersedes A4): +The `tail` is bounded (max ~10 chars), so streaming UX latency is at +most 10 chars worth of buffering when between fenced blocks. The +existing `assistant_delta`'s `stream_buf` for full-text accumulation +is unaffected — the filter sits BEFORE `emit`. + +`highlighted(body, lang)` — **B3 + R2 + R4-revised**: + +Lives in `repl.lua` (per R2; `renderer.lua` calls it via the +`highlight_fn` passed to `renderer.set_highlight`). Has access to +`_shq` (existing helper from #3) and the `executor` require. ```lua -if not highlight_enabled or not lang_map[lang] then return body end -local tmp = os.tmpname() -local f = io.open(tmp, "wb") -if not f then return body end -f:write(body); f:close() --- B3: LuaJIT io.popen():close() returns (true, nil, nil) regardless of --- exit code. Route via executor.exec which uses pty.spawn+waitpid and --- returns (out, exit_code) reliably. -local out, code = executor.exec( - ("cat %s | tree-sitter highlight --lang %s") - :format(_shq(tmp), lang_map[lang])) -os.remove(tmp) -if code ~= 0 then return body end -- pass-through on highlighter failure -return out +-- repl.lua local. Wired into renderer via: +-- renderer.set_highlight(true, treesitter_present, highlighted) +local function highlighted(body, lang) + if not highlight_enabled or not lang_map[lang] then return body end + + -- R4: tree-sitter highlight CLI grammar is UNVERIFIED. + -- Upstream `tree-sitter highlight` canonically takes a path and + -- infers language from the file extension. At commit-5 implement + -- time, install tree-sitter and check whether `--lang` exists. + -- If not, name the tmpfile with the language's canonical extension + -- (lang_extension[lang]) and pass the path directly: + -- tmp = os.tmpname() .. lang_extension[lang] + -- cmd = "tree-sitter highlight " .. _shq(tmp) + -- Below is the optimistic --lang form for code reading; the actual + -- implementation must be verified. + + local tmp = os.tmpname() + local f = io.open(tmp, "wb") + if not f then return body end + f:write(body); f:close() + -- B3: io.popen():close() doesn't expose exit codes in LuaJIT. + -- Route via executor.exec which uses pty.spawn+waitpid and + -- returns (out, exit_code) reliably. + local out, code = executor.exec( + ("cat %s | tree-sitter highlight --lang %s") + :format(_shq(tmp), lang_map[lang])) + os.remove(tmp) + if code ~= 0 then return body end -- pass-through on highlighter failure + return out +end ``` Why this shape (and not the formulate-time A4 sketch): -- **A4 assumed Lua 5.2+ popen-close-with-code.** LuaJIT (5.1 contract) - doesn't expose the exit status via `io.popen(...):close()`. Baseline - B3 caught this; the only reliable exit-code path in our substrate - is `executor.exec` (pty.spawn + waitpid). +- **R2 file placement**: `highlighted` lives in `repl.lua` so it has + natural access to `_shq` + `executor`. `renderer.lua` stays free of + the `executor` require; it calls back through `highlight_fn`. +- **B3 exit-code path**: LuaJIT (5.1 contract) doesn't expose the exit + status via `io.popen(...):close()`. `executor.exec` is the only + reliable channel in our substrate. +- **R4 grammar verification**: the `--lang` flag is the formulate-time + assumption; the upstream CLI's `highlight` subcommand may want a + PATH with a recognized extension instead. Implement-time check + required before commit 5 ships. - The tmpfile stays — avoids ARGMAX on `printf '%s' BODY |` and sidesteps shell-escape edge cases on arbitrary code-block bytes. -- Cost is one syscall round (tmpfile create/remove) plus one pty - spawn per code block — negligible vs the highlighter latency. -- `_shq` is the existing shell-quote helper from #3 (pre/post hooks). +- Cost: one syscall round (tmpfile create/remove) + one pty spawn per + code block — negligible vs the highlighter latency. ### Lang map (v1) @@ -298,11 +455,21 @@ Default: off (don't change existing-user UX). ### Meta: `:diff [args]` - `:diff` → `git diff` (working tree vs index) -- `:diff staged` → `git diff --cached` - `:diff HEAD` → `git diff HEAD` +- `:diff --cached` → `git diff --cached` (staged-only) - `:diff main..feature` → `git diff main..feature` - `:diff ` → passed verbatim to `git diff ` +N3: the meta is a thin pass-through to `git diff`. Don't introduce +aliases like `staged` that would diverge from git's own grammar — the +user types the real flag (`--cached`) and aish doesn't second-guess. + +R6: `:diff` reads `libc.getcwd()` at **meta-invocation** time. Compare +with `:tree` / `ctx.project` which captures the cwd at **scan** time +(A8): after `cd /other-project`, `:diff` shows the new project's diff, +but `ctx.project` still holds the old project's tree until `:tree +refresh`. + Implementation — **B1-revised** (must disable pager + color): ```lua @@ -387,10 +554,16 @@ Tiered resolution semantics: ### Meta: `:tree [depth]` -- `:tree` → scan + inject with default depth and char cap -- `:tree ` → override depth for this scan -- `:tree refresh` → re-scan with cached opts -- `:tree off` → clear `ctx.project` +- `:tree` → scan + inject with default depth and char cap; if a + prior `:tree ` set a depth override, this re-scan uses the + config defaults (`:tree` resets to defaults) +- `:tree ` → override depth for this scan; cached as + `ctx._project_opts` for `:tree refresh` +- `:tree refresh` → re-scan with `ctx._project_opts` (last explicit + opts) if present; otherwise config defaults (R7) +- `:tree off` → clear `ctx.project` AND `ctx._project_opts`. Future + `:tree` (no arg) re-scans with config defaults. One-shot semantics + — there's no "disabled until re-enabled" flag (R5). ### Scan logic @@ -401,10 +574,11 @@ local function _scan_project_tree(dir, opts) local depth = opts.depth or 3 -- Prefer git ls-files for .gitignore honor; fall back to find. - local in_git = os.execute("cd " .. shq(dir) .. " && git rev-parse --git-dir >/dev/null 2>&1") == 0 + -- N4: `git -C ` skips the subshell vs `cd && git ...`. + local in_git = os.execute(("git -C %s rev-parse --git-dir >/dev/null 2>&1"):format(shq(dir))) == 0 local listcmd if in_git then - listcmd = ("cd %s && git ls-files --cached --others --exclude-standard"):format(shq(dir)) + listcmd = ("git -C %s ls-files --cached --others --exclude-standard"):format(shq(dir)) else listcmd = ("find %s -maxdepth %d -type f -not -path '*/\\.*' 2>/dev/null"):format(shq(dir), depth + 1) end @@ -582,7 +756,16 @@ green (existing tests pass + smoke ok) and adds a discrete capability. `compose_summary` so the read order is memory → project tree → earlier-summary → NORRIS. Suppressed under `self.norris_active` (parity with R-C1 / R-C4). No behavior change yet — nothing sets - `ctx.project`. Smoke: `:to_messages()` still empty when project nil. + `ctx.project`. + + **R8: `:reset` does NOT clear `ctx.project`.** Phase 4 established + that `:reset` preserves `ctx.memory_items` (startup-injected facts + survive a user-driven context reset); `ctx.project` follows the + same rule. Compare `Context:reset` at `context.lua` ~343 — clears + `turns`, `pending_exec_output`, `summary`; leaves `memory_items` + and now `project` alone. Smoke: `:to_messages()` still empty when + project nil; with project set, `:reset` then `:to_messages()` + still shows the `[project]` block. 2. **`repl.lua` — `_scan_project_tree` helper + `:tree` meta.** - `_scan_project_tree(dir, opts)` per §6: `git ls-files --cached @@ -629,24 +812,31 @@ green (existing tests pass + smoke ok) and adds a discrete capability. b. `renderer.lua` — fence-aware state machine wrapping `assistant_delta`. Exports `renderer.set_highlight(enabled, - detected)` so repl.lua wires the toggle + cli-availability - flags. State: `outside` (pass-through) / `inside` (buffer until - closing fence). On close: call `highlighted(body, lang)` and - emit. Algorithm per §4; bytes-of-cumulative-buf scan as B2 - requires for fragment-across-boundary fences. + detected, highlight_fn)` so repl.lua wires the toggle, + cli-availability flag, AND the `highlighted` callback (R2: + keeps `executor` dependency out of `renderer.lua`). State: + `outside` (pass-through + tail accumulator per R1) / + `inside` (buffer until closing fence). On close: call + `highlight_fn(body, lang)` and emit. Algorithm per §4; + bytes-of-cumulative-buf scan + tail lookahead handles + fragment-across-boundary fences (B2 + R1). - c. `highlighted(body, lang)` per §4 (B3-revised): write body to - `os.tmpname()`, invoke via `executor.exec("cat tmp | - tree-sitter highlight --lang X")`, capture out + exit code, - cleanup tmp, pass-through on failure. + c. `highlighted(body, lang)` per §4 (B3 + R2 + R4): lives in + `repl.lua`. Write body to `os.tmpname()`, invoke via + `executor.exec("cat tmp | tree-sitter highlight --lang X")`, + capture out + exit code, cleanup tmp, pass-through on failure. + **R4 implement-time check**: verify the `--lang` flag exists + on the installed CLI; if not, switch to tmpfile-with-extension + and pass the path directly. d. `:highlight [on|off|status]` meta in repl.lua. `:highlight on` when CLI absent → status with install hint (B4); `:highlight status` always reports current toggle + CLI availability. - e. HELP update; PHASE6 status → Implement. + e. HELP update. **R9: status header bump moves to commit 6** + (single owner; no duplication). -6. **`config.lua` + docs/PHASE6 status bump.** +6. **`config.lua` + docs/PHASE6 status bump (R9).** - Add commented-out `project = { auto_tree = false, tree_depth = 3, tree_max_chars = 4096 }` block in config.lua (parity with the Phase 1-5 example blocks). @@ -661,9 +851,11 @@ green (existing tests pass + smoke ok) and adds a discrete capability. | 2 (:tree) | `find` fallback picks up node_modules / target / build / etc. | Document in status warning; users in non-repo cwds scope via `:tree ` | | 3 (:diff) | B1 — color/keypad codes leak if a future caller forgets the helper | All call sites must go through `_git_clean_cmd`; lint by grep before commit | | 4 (@..) | False positive on `@../sibling.txt` when no such file exists | A6's tiered resolution: only retry as diff when file lookup fails. `@../sibling.txt` resolves as path; if the path is missing, diff retry runs and naturally fails — same outcome as before | -| 5 (highlighter) | Fence detector misclassifies inline ` ` ``` ` ` triple-backtick in prose | State machine triggers on `^```` at the start of a line OR following a newline only; mid-line backticks don't open a fence. Document in §4. | +| 5 (highlighter) | Fence detector misclassifies inline ` ` ``` ` ` triple-backtick in prose | N1: state machine triggers on `^```` at start of stream OR after `\n` only. §4 algorithm now encodes this constraint in the pseudocode. | | 5 (highlighter) | tmpfile race / leak on crash | `os.remove(tmp)` in normal exit path; OS cleans `/tmp/lua_*` files on reboot. Single-user trust per PHASE0 §12. | -| 6 (config bump) | none — pure docs / commented config | +| 5 (highlighter) | R3: PTY raw-mode toggle on every code-block render (`executor.exec` -> `libc.set_raw(0)`) | Smoke-test before locking: render an assistant turn with 5+ fenced blocks; watch for cursor flicker, SIGWINCH races, terminal state corruption. If problematic, alternate paths: direct `io.popen` for stdin-write (accept the lost exit code; treat empty output as failure) or run highlighter via `os.execute` with shell redirection. | +| 5 (highlighter) | R4: `tree-sitter highlight --lang X` invocation grammar unverified | Implement-time CLI check (`tree-sitter highlight --help`). If `--lang` is wrong, fall back to extension-based: name the tmpfile `lua_XXX.` per `lang_extension[lang]` map and pass the path. | +| 6 (config bump + status) | none — pure docs / commented config | ### Tests + smoke per commit @@ -692,10 +884,13 @@ inline `luajit -e '...'` blocks in commit messages or as a dedicated ### Open at plan-time (resolve at implement) -- Whether `_scan_project_tree` should honor a per-call `opts.dir` - override (so a future feature like "scan ``" lands cheaply) - vs hardcoding `libc.getcwd()`. Default to taking `dir` as arg; - the `:tree` meta passes `libc.getcwd()` explicitly. +- **R4 implement-time verification**: confirm `tree-sitter highlight + --lang X` works on the installed CLI. If not, switch to extension- + based path passing. Block commit 5 ship on this check. +- **R3 smoke test**: render an assistant turn with 5+ fenced blocks + through the highlighter; confirm no cursor flicker / SIGWINCH race + / terminal-state corruption from per-block raw-mode toggle. If + problematic, alternate paths listed in §12 risk row. - Whether `:highlight status` should also probe `tree-sitter --print-langs` to show which langs are actually available. Nice-to-have; defer unless install paths produce variable lang sets in practice.