leanos/REVIEW.md

# Code Review — NEON NOODLE OS

Five-pass review. One pass per loop iteration. Findings: file, line, severity, fix.
Critical/high issues are fixed directly; medium/low are logged for triage.

| Pass | Topic | Status |
|------|-------|--------|
| 1 | Runtime / type / broken-import errors | ✅ done |
| 2 | Logic bugs (races, edge cases, null checks) | ✅ done |
| 3 | Security audit | ✅ done |
| 4 | Error handling | ✅ done |
| 5 | Consistency (naming, dead code, duplication) | ✅ done |

---

## Pass 1 — Runtime / type / broken-import errors

**Method:** `npx tsc --noEmit` (strict), production `vite build`, import-resolution scan,
manual read of the runtime-critical path (`appRuntime.tsx` Babel transform + `new Function` scope).

**Result: no runtime, type, or import errors.**

- `tsc --noEmit` — clean, zero diagnostics.
- `vite build` — succeeds (only the pre-existing >500 kB chunk-size advisory, not an error).
- No project ESLint config present (ESLint not configured) — noted, not a defect.
- All `import` specifiers resolve to existing modules; no dangling paths.
- `appRuntime.compileApp` passes React + hooks + `os` into a `new Function` scope and
  memoises the built component per `os` instance — no obvious runtime fault in the happy path.

### Findings
_None at severity high or above. Observations deferred to later passes:_

| # | File:line | Sev | Note | Defer to |
|---|-----------|-----|------|----------|
| 1.1 | `kernel/appRuntime.tsx:223` | low | Compile `cache` Map is never evicted; entries accumulate across self-heals (bounded by app count × versions). | Pass 2 |
| 1.2 | `kernel/ai.ts:62` | low | `currentProvider()` exported but unused by callers (Settings reads `aiProvider` from the store). Possible dead export. | Pass 5 |

No fixes applied in Pass 1 (nothing at critical/high).

---

## Pass 2 — Logic bugs (races, edge cases, off-by-one, null checks)

**Method:** manual read of state/logic paths — `store.ts` (window mgr, openApp,
installAndOpen), `fs.ts`, `sdk.ts`, `bus.ts`, `appRuntime.tsx`, `AppHost.tsx`,
`Window.tsx` (drag/resize/snap), `Terminal.tsx`, `Assistant.tsx`.

### Findings

| # | File:line | Sev | Issue | Fix |
|---|-----------|-----|-------|-----|
| 2.1 | `apps/Terminal.tsx:84` (rm) | **high** | `resolve("")` returns `cwd`, so a bare `rm` (no operand) deletes the **current directory** — at `/` it wipes the entire virtual FS. Also affected `cat`/`mkdir`/`touch` (operated on cwd silently). | ✅ **Fixed** — added missing-operand guards to `rm`, `cat`, `mkdir`, `touch`. |
| 2.2 | `kernel/sdk.ts:32` | **medium** | `ai.available` was a one-time snapshot (`aiAvailable()`) captured when the window's `os` API is built (memoised per window in `AppHost`). Configuring the key in Settings afterwards left already-open apps (e.g. Terminal) reporting AI offline. | ✅ **Fixed** — converted `available` to a getter returning the live value. |
| 2.3 | `system-ui/Window.tsx:33` | low | Title-drag clamps Y to below the menu bar but never clamps X — a window can be dragged fully off-screen left/right with no way back. | deferred (UX, low) |
| 2.4 | `kernel/store.ts:274` | low | `installAndOpen` fuzzy match: `a.name.toLowerCase().includes(query)` can match the wrong app on very short queries (e.g. "a"). Intended fuzzy behaviour; acceptable. | deferred (low) |
| 2.5 | `kernel/fs.ts:65` (write) | low | `write` only auto-creates the parent if it's absent; if the parent path exists as a **file**, the child is written anyway (no "not a directory" error, unlike `mkdir`). | deferred (low) |
| 2.6 | `kernel/appRuntime.tsx:223` | low | (from Pass 1) compile `cache` Map never evicted. Bounded by app×version count; not a leak in practice. | deferred (low) |

**Fixes applied:** 2.1 (high), 2.2 (medium). Typecheck clean after changes.

---

## Pass 3 — Security audit

**Method:** `npm audit`, dangerous-sink grep (`innerHTML`, `dangerouslySetInnerHTML`,
`eval`, `new Function`, `document.write`), secret-handling trace (`ai.ts`/`sdk.ts`/`db.ts`),
sandbox/blocklist review (`appRuntime.tsx`), dev-proxy review (`vite.config.ts`), `.gitignore`.

**Good:**
- `npm audit` (prod deps) — **0 vulnerabilities**.
- No `innerHTML` / `dangerouslySetInnerHTML` / `document.write` anywhere — React auto-escaping intact; Terminal renders into `<pre>` (safe).
- `.env`, `.env.local` are git-ignored — secrets not committed.
- API keys are the user's own and sent only to the selected provider (correct auth headers per API shape).

### Findings

| # | File:line | Sev | Issue | Fix |
|---|-----------|-----|-------|-----|
| 3.1 | `kernel/appRuntime.tsx:25` | **medium** | Generated apps run via `new Function` in the page realm; the regex blocklist omitted `indexedDB`, so app code could open the `fluid-os` DB and read the `settings → aiConfig` store (all API keys) and exfiltrate them. | ✅ **Fixed** — added `indexedDB`, `globalThis`, `Function(`, `import.meta` to the blocklist (defense-in-depth). |
| 3.2 | `kernel/appRuntime.tsx:42` | medium | The blocklist is **not** a real boundary — generated code shares the realm and can still escape via deep prototype tricks (`[].constructor.constructor`, etc.). Documented inline. | accepted by design (PROMPT.md: in-page execution). True isolation = iframe/worker — large refactor, deferred. |
| 3.3 | `vite.config.ts:90` (`/__ai/proxy`) | low (dev-only) | Dev proxy forwards to any `x-llm-url` with the request's auth headers — an open proxy / SSRF + key-relay vector. **Not present in production builds** (no middleware ships). | accepted (dev convenience; required for multi-provider CORS bypass). |
| 3.4 | `vite.config.ts:9` (`/__proxy`) | low (dev-only) | In-OS browser proxy fetches arbitrary `?url=` server-side and strips framing headers — SSRF in dev only. | accepted (dev-only; intended for the browser app). |
| 3.5 | `kernel/db.ts` (settings store) | low/info | API keys persisted in IndexedDB in plaintext — inherent to a backend-less client app; keys never leave except to the chosen provider. Mitigated against in-page theft by 3.1. | accepted by design. |
| 3.6 | `kernel/ai.ts:100` | info | `anthropic-dangerous-direct-browser-access: true` enables direct browser calls to Anthropic — intentional for local dev. | accepted. |

**Fixes applied:** 3.1 (medium — blocklist hardening). Typecheck clean.

---

## Pass 4 — Error handling (uncaught exceptions, missing try/catch, silent failures)

**Method:** traced every async path and entry point — `store.init/installAndOpen/healApp`,
`App.tsx` boot, `sdk.ts` fs/ai, built-in apps (`Terminal`, `Files`, `Assistant`, `AppBuilder`),
`ai.ts` chat/stream, the render-time `AppErrorBoundary`.

**Already solid:**
- `installAndOpen`, `healApp` (store) — try/catch with user-facing `notify`. ✓
- `chat` / `chatStream` (ai.ts) — throw with provider + status + body on non-2xx; callers catch. ✓
- `Terminal.run`, `Assistant.send` — try/catch surfacing errors to the UI. ✓
- `AppErrorBoundary` + self-heal catches **render** errors in generated apps. ✓
- `bus.emit` — wraps each handler in try/catch. ✓

### Findings

| # | File:line | Sev | Issue | Fix |
|---|-----------|-----|-------|-----|
| 4.1 | `kernel/store.ts:100` + `App.tsx:53` | **high** | `init()` had no try/catch and `App` calls `void init()`. Any failure in `fs.seed`/`loadAiConfig`/`loadApps` (e.g. IndexedDB blocked in private mode) left `ready=false` → **OS stuck on the boot screen forever, silently** (rejection swallowed by `void`). | ✅ **Fixed** — `init()` now try/catches: on failure it boots with built-in apps + in-memory defaults, sets `ready=true`, and notifies "Storage unavailable". OS always reaches the desktop. |
| 4.2 | `apps/Files.tsx:13,22,32,44,54` | low | `refresh/open/save/create/del` call `os.fs.*` without try/catch; an IndexedDB failure becomes an unhandled rejection (console-only, no UI feedback). Error boundary doesn't catch async. | deferred (low — IndexedDB ops reliable; built-in app). |
| 4.3 | `apps/Settings.tsx` `onSave` | low | `await saveAiConfig(cfg)` not wrapped; a persistence failure would skip `setSaved(true)` and reject unhandled. | deferred (low). |
| 4.4 | `kernel/store.ts` `setTheme`/`setWallpaper` | low | `void setSetting(...)` fire-and-forget swallows persistence errors silently (cosmetic settings only). | accepted (low impact). |
| 4.5 | `apps/Terminal.tsx` (ai stream) | low | If `os.ai.stream` throws mid-stream, the catch reports it but the pre-pushed empty "out" line remains. Cosmetic. | deferred (low). |

**Fixes applied:** 4.1 (high — boot resilience). Typecheck clean.

---

## Pass 5 — Consistency (naming, dead code, duplicated logic)

**Method:** export-usage grep (dead code), stale-string scan, magic-number/duplication scan.

### Findings

| # | File:line | Sev | Issue | Fix |
|---|-----------|-----|-------|-----|
| 5.1 | `kernel/ai.ts:62` | low | `currentProvider()` exported but never imported anywhere (Settings reads `aiProvider` from the store). Dead code. | ✅ **Fixed** — removed. |
| 5.2 | Spotlight:148, Assistant:43, AppBuilder:33, Terminal:109, store:327 | medium | Five user-facing strings still said "set VITE_ANTHROPIC_API_KEY in .env" — stale since keys moved to **Settings → Language Model** (multi-provider). Misleading/inconsistent UX. | ✅ **Fixed** — all reworded to "add an API key in Settings → Language Model". |
| 5.3 | `store.ts:78` & `Window.tsx:8` | low | `MENUBAR_H = 36` duplicated as a magic number in two modules (plus `DOCK_RESERVE`, `MIN_W/H`). | deferred — extract to a shared `layout` constants module (low risk, low value). |
| 5.4 | `App.tsx:12` `cssVars` vs `tokens.ts` `tokensAsCss` | low | Two overlapping CSS-var builders. Intentional (apps get a subset; root gets the full set incl. shadows/radius-xl) but the key list is partly duplicated. | accepted (different scopes by design). |
| 5.5 | `registry.ts` icons | low | App Builder uses "⚙️" and Settings uses "⚙" — near-identical gears, visually ambiguous in dock/desktop. | deferred (cosmetic identity choice — recommend a distinct glyph for App Builder, e.g. 🛠️). |

**Fixes applied:** 5.1 (dead code removed), 5.2 (medium — stale strings). Typecheck + full `vite build` clean.

---

# Summary

All 5 passes complete. Verified with `tsc --noEmit` (clean) and production `vite build` (succeeds).

**Fixes applied (7):**

| Sev | Pass | Fix |
|-----|------|-----|
| **high** | 2.1 | `Terminal` `rm`/`cat`/`mkdir`/`touch` now require an operand — a bare `rm` no longer deletes the cwd / whole virtual FS. |
| **high** | 4.1 | `store.init()` is now fault-tolerant — IndexedDB failure boots the OS with defaults + a notification instead of hanging on the boot screen forever. |
| **medium** | 2.2 | `os.ai.available` is a live getter, not a stale snapshot — apps see the key after it's configured in Settings. |
| **medium** | 3.1 | Sandbox blocklist hardened (`indexedDB`, `globalThis`, `Function(`, `import.meta`) — closes the trivial API-key exfiltration path. |
| **medium** | 5.2 | Five stale "VITE_ANTHROPIC_API_KEY" prompts reworded to point at Settings → Language Model. |
| low | 5.1 | Removed dead `currentProvider()` export. |

**Notable accepted/deferred items:**
- **3.2 (medium):** generated-code sandbox is not a true security boundary (in-page `new Function`). Accepted by design (PROMPT.md); real isolation needs an iframe/worker — a substantial refactor, out of scope for this review.
- **3.3/3.4 (low):** dev-server proxies are open-proxy/SSRF vectors but ship only in dev, never in production builds.
- Low-severity polish (window X-clamp, Files/Settings async try/catch, MENUBAR_H dedupe, duplicate gear icons) logged above for future triage.

**Overall:** healthy codebase. 0 dependency vulnerabilities, no type/import errors, no XSS sinks. The two highest-impact issues (FS-wiping `rm`, silent boot hang) are fixed; the chief residual risk (sandbox isolation) is inherent to the product's design and documented.

_Review complete — loop stopped._