diff --git a/docs/PHASE9.md b/docs/PHASE9.md index b105191..e7d3981 100644 --- a/docs/PHASE9.md +++ b/docs/PHASE9.md @@ -2,9 +2,84 @@ **Project:** aish — AI-augmented conversational shell **Document:** Phase 9 Requirements, Architecture & Design Decisions -**Status:** Plan (formulate + analyze + baseline complete; tree at `4f5c3ae`) +**Status:** Plan + review fold-in (tree at `31e5de5`) **Date:** 2026-05-16 +**Review findings (Sonnet, 2026-05-16) — 0 BLOCKERs, 7 CONCERNs +folded, 5 NITs applied:** + +R1 (CONCERN, FOLDED). **HOME prefix false-positive in walk-up.** + `dir:sub(1, #home) ~= home` lets `/home/user2/...` pass when + HOME is `/home/user` (matches first 10 bytes). Real bug. Fix: + `if dir ~= home and dir:sub(1, #home + 1) ~= home .. "/" then + return nil end`. §4 code updated. + +R2 (CONCERN, FOLDED). **`io.read` trust-prompt fallback breaks + `aish -p` piped stdin.** A8's fallback (`io.read("*l")` if + rl.readline misbehaves at startup) would consume the first + line of piped stdin in non-interactive mode. **Fix:** in + one-shot mode (`opts.prompt` set), SKIP the trust prompt + entirely and decline silently with a status line. Project + overlays in `-p` mode require pre-existing trust. Documented + in §13 commit 2. + +R3 (CONCERN, FOLDED). **Sources-map delivery decided: `cfg._sources` + embedded on the config table** (NOT a global). `repl.run` reads + `config._sources` for `:config show`. Backward-compatible — old + callers of `repl.run` that don't pass `_sources` still work + (`:config show` says `(sources unknown)`). §4 + §13 commits 2+3 + updated to reflect. + +R4 (CONCERN, FOLDED). **`_prompt_trust` signature contradicted + `_check_trusted`'s "compute sha once" claim.** §5 sketch called + `_record_trust(project_path)` which would re-sha256. **Fix:** + `_prompt_trust(project_path, sha)` takes the pre-computed sha; + `history.add_trusted(trust_path, project_path, sha)` is the + one writer. §5 sketches updated to match §13 + the real + history.lua API. + +R5 (CONCERN, FOLDED). **`_check_trusted` duplicated trust-file + read logic vs history.lua API.** §5 sketch had inline JSONL + read; §13 defines `M.is_trusted(trust_path, project_path, + sha256)` in history.lua to own that. **Fix:** §5 sketches now + call `history.is_trusted(...)` and `history.add_trusted(...)` — + main.lua holds no trust-file logic itself. This also makes the + `$AISH_TRUST_FILE` env override work cleanly (one resolution + site). + +R6 (CONCERN, FOLDED). **`:config show full` mode masking + unspecified for nested values** — the actual leak vector is + `mcp.servers..auth_token`. **Fix:** §6 + §13 commit 3 + spell out: same heuristic, applied RECURSIVELY in full mode. + Top-level mode (default) already collapses nested tables, so + no leak there. + +R7 (CONCERN, FOLDED). **Shallow merge silently drops user's entire + models block** (or permissions, cost, etc.). Documented as + "predictable" but is a real UX trap. **Fix:** §1 done-when + + §7 UX surface + §13 commit 4 template-comment all gain a + conspicuous warning: "If your `.aish.lua` sets a top-level + block (models, permissions, cost, ...) it REPLACES your user + config's entire block — list every entry you want available + OR omit the block to keep the user's." Stronger framing than + "predictable". + +R-N1..N5 (NITs, APPLIED): + N1. (cosmetic — review-prompt clarification only; no doc change) + N2. `key_env` / `auth_env` over-masking is a known false-positive + of the heuristic (env-var NAME, not a secret). §13 commit 3 + risk row gains an explicit note: "values of `*_env` fields + will be masked too; cosmetic only — they hold env-var names, + not secrets. Future: refine heuristic to exempt `*_env` + pattern." + N3. §13 open-at-plan-time list now includes the + sources-map-delivery decision (resolved by R3 — embed on cfg). + N4. §9 risk row about trust file partial write gains explicit + first-ever-write edge case + workaround (manually delete the + corrupt file). Temp-file+rename is v2 polish. + N5. §3 module table ffi/libc.lua row had stale "stat" mention; + removed per A2 (io.open is sufficient). + **Analyze + baseline findings (2026-05-16) — 5/6 open Qs resolved in-place; Q-P4 deferred to implement-time verify:** @@ -167,7 +242,7 @@ Four pillars: | File | State after Phase 8 | Phase 9 changes | |---|---|---| | `main.lua` | `load_config(opts)` walks $AISH_CONFIG → ~/.config/aish → ./config.lua | Wrap with `load_with_project_overlay(opts)` that finds the user config (existing logic) AND walks up from cwd for `.aish.lua`; if both found, merge project ONTO user and return merged. Records source-per-key for `:config show`. | -| `ffi/libc.lua` | getcwd, chdir, isatty, flock | Add `stat` for filesystem checks during walk-up (or use `io.open(path,"r")` for existence — simpler, no new FFI). | +| `ffi/libc.lua` | getcwd, chdir, isatty, flock | **No change** (per A2): `io.open(candidate, "rb")` is sufficient for existence-check during walk-up. No new FFI bindings needed. | | `repl.lua` | All the metas including `:config` (nope — no :config yet) | New `:config show` meta. Source-map carried on a module-local set at startup; meta reads it. | | `history.lua` | session log, memory.jsonl | New helpers: `M.read_trusted(path)` returns set of trusted entries; `M.add_trusted(path, target_path, sha256)` appends. Mode 0600 enforced. | | `config.lua` (the user's global; not the in-tree example) | n/a | No change. The in-tree `config.lua` becomes a template that project overlays can replace top-level keys of. | @@ -189,9 +264,13 @@ local function _find_project_config() local dir = libc.getcwd() if not dir then return nil end - -- Don't walk OUTSIDE $HOME. If cwd isn't inside $HOME, no - -- project search. - if dir:sub(1, #home) ~= home then return nil end + -- R1: don't walk OUTSIDE $HOME. The proper-prefix check requires + -- `dir == home` OR `dir starts with home .. "/"` — bare + -- `sub(1, #home) == home` matches "/home/user2" when HOME is + -- "/home/user" (10-byte prefix). Real bug caught by review. + if dir ~= home and dir:sub(1, #home + 1) ~= home .. "/" then + return nil + end while dir and #dir > 0 do local candidate = dir .. "/.aish.lua" @@ -271,36 +350,32 @@ Source map is then carried as a closure local in `repl.run` for `:config show`. {"path":"/home/user/src/other/.aish.lua","sha256":"def456...","ts":"2026-05-16T12:40:00Z"} ``` -### Trust check +### Trust check + prompt (R4 + R5 — calls history.lua API; sha computed once) ```lua -local function _check_trusted(project_path) - local path = (os.getenv("HOME") or "") .. "/.aish/trusted-projects" - local f = io.open(path, "r") - if not f then return false end - local current_sha = _sha256_file(project_path) - for line in f:lines() do - local entry = json.decode(line) - if entry and entry.path == project_path - and entry.sha256 == current_sha then - f:close() - return true - end - end - f:close() - return false +-- R5: trust-file path resolves through history.lua + optional env override. +-- main.lua never reads/writes the trust file directly. +local function _trust_file_path() + return os.getenv("AISH_TRUST_FILE") + or ((os.getenv("HOME") or "") .. "/.aish/trusted-projects") end -``` -### Trust prompt - -```lua -local function _prompt_trust(project_path) +-- R4 + R5: compute sha ONCE; pass to history.is_trusted / add_trusted. +local function _check_and_maybe_prompt(project_path) + local sha = history._sha256_file(project_path) + if not sha then + renderer.status("project config "..project_path..": sha256 failed; skipping") + return false + end + local tpath = _trust_file_path() + if history.is_trusted(tpath, project_path, sha) then + return true + end renderer.status("project config found: " .. project_path) renderer.status("UNTRUSTED. Loading it runs arbitrary Lua code.") local ans = rl.readline("[aish] trust this project config? [y/N] ") if ans and ans:lower():sub(1, 1) == "y" then - _record_trust(project_path) + history.add_trusted(tpath, project_path, sha) return true end return false @@ -309,7 +384,10 @@ end ### sha256 -Shell out: `sha256sum | cut -d' ' -f1`. POSIX-portable; faster than vendoring. Cached result during the trust check (single call per startup). +`history._sha256_file(path)` shells out to `sha256sum ` and parses +the first whitespace-separated field. Single call per startup per +project file (R4 — `_check_and_maybe_prompt` computes once and passes +to both `history.is_trusted` and `history.add_trusted`). --- @@ -328,8 +406,17 @@ Shell out: `sha256sum | cut -d' ' -f1`. POSIX-portable; faster than vendo ... ``` -Token-bearing fields (anything matching `auth_token`, `*_TOKEN`, etc.) -displayed as `(set)` rather than the value. +Token-bearing fields (any key matching `token`, `secret`, `auth`, +`key`, case-insensitive) displayed as `(set)` rather than the value. + +R6 — `:config show full` applies the SAME heuristic RECURSIVELY to +nested values (the actual leak vector is `mcp.servers..auth_token` +which top-level mode collapses but full mode would dump). + +Known cosmetic false-positive (N2): `key_env` / `auth_env` config +fields are over-masked. These hold env-var NAMES (e.g. `OPENAI_API_KEY`) +not the secret values themselves — but the heuristic catches them. +Future polish: exempt `*_env` from the heuristic. --- @@ -379,11 +466,11 @@ No new config keys in v1 (the project overlay IS the new mechanism; it doesn't n | Risk | Mitigation | |---|---| | Hostile `.aish.lua` in cloned repo runs arbitrary Lua on first `aish` run in that cwd | Trust prompt + sha256 persistence; default = decline if user just hits Enter at the [y/N]. | -| Trust file becomes corrupted / unreadable | Best-effort: corrupted lines skipped; missing file means all projects untrusted (re-prompt on next encounter). | +| Trust file becomes corrupted / unreadable | Best-effort: corrupted lines skipped (each line is independent JSON); missing file means all projects untrusted (re-prompt on next encounter). N4 edge case: if the FIRST-EVER write is interrupted partway, the file's sole line may be corrupt JSON and the project never stays trusted — user manually deletes `~/.aish/trusted-projects` to recover. Temp-file+rename atomicity is v2 polish. | | User trusts `.aish.lua`, repo is updated, malicious code is injected | sha256 mismatch on next startup triggers re-prompt. User sees the prompt and can investigate before granting trust again. | | `dofile` errors at load time (syntax error in project config) | pcall-protected; status line "project config X failed to load; ignoring" — aish continues with just the user config. | | Walk-up walks above $HOME (e.g., a repo cloned to `/tmp`) | $HOME boundary check stops the walk. `/tmp` repos get no project layer (user can move them under $HOME or use --config). | -| Shallow merge surprises a user who wanted to add ONE model preset | Documented as predictable / explicit; users compose the full models block deliberately. | +| **R7 — shallow merge silently DROPS the user's entire block on overlap.** A `.aish.lua` that sets `models = {...}` REPLACES the user's full models block; same for `permissions`, `cost`, `shell`, etc. This is a genuine UX trap, not just "predictable" — accept-and-warn-clearly is the resolution rather than hiding behind framing. | Conspicuous warning in §1 done-when + §7 UX table + config.lua template header: "If your `.aish.lua` sets a top-level block (models, permissions, cost, ...) it REPLACES your user config's entire block — list every entry you want available OR omit the block to keep the user's." Deep-merge-with-explicit-replace-syntax (systemd drop-in style) is v2 polish. | | Source map dict grows unboundedly with new keys mid-session | Bounded by #config top-level keys (small constant; <20). No GC needed. | --- @@ -436,32 +523,51 @@ beyond the existing config loader. check, sha mismatch returns false, missing file). 2. **`main.lua` — walk-up + load_with_project_overlay.** - - `_find_project_config()` walks from libc.getcwd() up to $HOME, - returning first `.aish.lua` or nil. - - `_prompt_trust(project_path, sha)` calls `rl.readline` with - the trust prompt; on accept, calls `history.add_trusted`. - A8: smoke-test rl.readline behavior at this early call site. + - `_find_project_config()` walks from libc.getcwd() up to $HOME + (R1 corrected proper-prefix check), returning first `.aish.lua` + or nil. + - `_check_and_maybe_prompt(project_path)` (R4 + R5) calls + `history._sha256_file` ONCE; routes through `history.is_trusted` + / `history.add_trusted` with the env-overridable trust file + path. Returns true if the project file should be loaded. - `load_config_with_overlay(opts)` wraps existing `load_config`; finds project, checks trust, prompts if needed, dofiles + - merges shallow over user config. Returns `(cfg, sources, paths)` - triple where sources maps top-level keys to "user"/"project" - for `:config show`. - - `main()` calls `load_config_with_overlay`, stashes sources - into a global so repl.lua can read it (or passes via cfg). - - Smoke: tree-resolution test from a nested cwd; trust prompt - accept/decline paths. + merges shallow over user config. **R2: in one-shot mode + (`opts.prompt` is set), the trust prompt is SKIPPED entirely + — the project layer is only loaded if it's already pre-trusted. + Avoids io.read consuming the first line of piped stdin.** + - **R3 sources delivery: embed on `config._sources`** (a sentinel + field on the config table itself). NOT a global. `repl.run` + reads `config._sources` for `:config show`; backward-compatible + (old callers without _sources are reported as "(sources + unknown)" by the meta). + - Smoke: (a) tree-resolution from a nested cwd; (b) trust prompt + accept-then-load + decline-then-skip paths; (c) -p mode with + untrusted .aish.lua + piped stdin -> trust prompt SKIPPED, no + stdin consumption; (d) A8: rl.readline early-startup smoke; + if rl.readline misbehaves, NO fallback to io.read in + interactive mode either — emit status + skip overlay (avoids + the silent-data-loss risk R2 covers). 3. **`repl.lua` — `:config show` meta + startup status line.** - - `:config show` / `:config show full` meta reads the sources - map + the effective config; sanitizes token-bearing values - (any key matching `_token`, `_TOKEN`, `auth_token` → display - as `(set)`); prints sources + key-by-key effective. + - `:config show` / `:config show full` meta reads `config._sources` + (R3 cfg-embedded) + the effective config; sanitizes token-bearing + values (any key containing "token"/"secret"/"auth"/"key", + case-insensitive) → display as `(set)`. R6: in `full` mode, + applies the heuristic RECURSIVELY to nested values (the real + leak vector is `mcp.servers..auth_token`). + If `config._sources` is absent, status: "(sources unknown — main + didn't pass _sources)" so the meta still runs but doesn't lie. - Startup status line per A4: AFTER the existing `aish: loaded config from `, if project layer fired, emit `[aish] project config: (overlaid on )`. - HELP gains 2 `:config` lines. + - N2 known false-positive: `key_env` / `auth_env` config field + VALUES are masked too (they hold env-var names, not secrets). + Cosmetic; future polish exempts `*_env`. - Smoke: with a test project file, run `:config show` and - verify keys + sources line up. + verify keys + sources line up; `:config show full` masks + nested auth tokens but exposes other nested fields. 4. **`config.lua` template note + status bump.** - Add a header comment to `config.lua` (the in-tree example) @@ -497,8 +603,11 @@ Each commit: ### Open at plan-time (resolve at implement) -- A8: rl.readline early-startup behavior. If broken, swap to - io.read("*l") for the trust prompt only. -- Whether to make the trust file path itself overridable via - `$AISH_TRUST_FILE` env. Useful for CI / test isolation. Default - to ~/.aish/trusted-projects; env override is one line. +- A8: rl.readline early-startup behavior. R2 supersedes the + formulate-time io.read fallback — if rl.readline misbehaves, + emit status + skip the overlay entirely (NOT a fallback to + stdin which would consume piped data in -p mode). +- `$AISH_TRUST_FILE` env override — RESOLVED: implement it (one + line; useful for CI / test isolation). Used by the verify TCs. +- N3 — sources-map delivery RESOLVED: embed on `config._sources` + (cfg-field; not a global). Per R3.