docs/PHASE9: review fold-in — 0 BLOCKERs + 7 CONCERNs + 5 NITs
Sonnet review of PHASE9 (formulate + analyze + baseline + plan at
31e5de5). No BLOCKERs (manifest design sound); seven real CONCERNs
including a path-prefix bug + a piped-stdin interaction that would
have surfaced at implement time.
CONCERNs (FOLDED):
R1. HOME-prefix walk-up false positive — dir:sub(1, #home) ~= home
matches /home/user2 when HOME=/home/user. Real bug. Fix:
`dir ~= home and dir:sub(1, #home + 1) ~= home .. "/"`.
R2. A8's io.read("*l") fallback for trust prompt would consume the
first line of piped stdin in aish -p mode. Fix: SKIP trust
prompt in one-shot mode (load only pre-trusted overlays).
If rl.readline misbehaves interactively, emit status + skip
overlay (no fallback to stdin in either mode).
R3. Sources-map delivery decided: cfg-embedded as config._sources.
Globals across module boundaries explicitly avoided. Backward-
compat: if absent, :config show reports "(sources unknown)".
R4. _prompt_trust signature fixed — takes pre-computed sha; single
sha256 call per startup per project file.
R5. _check_trusted no longer reimplements trust-file read logic;
routes through history.is_trusted / history.add_trusted with
AISH_TRUST_FILE env override (single resolution site).
R6. :config show `full` mode masking now spec'd: same heuristic
applied RECURSIVELY to nested values (mcp.servers.X.auth_token
is the actual leak vector).
R7. Shallow-merge UX trap reframed — was "documented as predictable";
now an explicit conspicuous warning in done-when + UX surface +
config.lua template that "if your .aish.lua sets a top-level
block, it REPLACES the user's entire block". Deep-merge with
explicit-replace-syntax v2 polish.
NITs (APPLIED):
N1. (no doc change — review-prompt clarification only)
N2. key_env / auth_env over-masking documented as known cosmetic
false-positive (env-var names, not secrets).
N3. Sources-map decision added to open-at-plan-time before
falling-into-commit-2 surprise.
N4. Trust-file first-write atomicity edge case documented (manual
delete to recover); temp-file+rename = v2.
N5. Stale "stat" mention in §3 module table removed (A2: io.open
is sufficient; no new FFI).
Code sketches in §4 + §5 + §6 + §13 commits 2+3 all updated to
reflect the fixes. Manifest is internally consistent + matches the
history.lua API to be added in commit 1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+163
-54
@@ -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.<alias>.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 <path> | 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 <path>` 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 <path> | 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.<alias>.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.<alias>.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 <path>`, if project layer fired, emit
|
||||
`[aish] project config: <path> (overlaid on <user>)`.
|
||||
- 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.
|
||||
|
||||
Reference in New Issue
Block a user