# Code Review — Technical Debt & Stability Issues

**Date:** 2026-04-29
**Scope:** Full backend + frontend review
**Total items:** 53 issues

Security items are listed in a separate deferred section at the end of this document. They are **not** in scope for the current fix pass but must be addressed before any multi-user or public-facing deployment.

---

## Critical — Stability / Data Loss / Crash

| # | Problem | File | Line | Explanation |
|---|---------|------|------|-------------|
| 1 | **[FIXED] Race condition: concurrent runs overwrite each other** | `navi/api/websocket.py` | 70, 328-331, 164 | `_runs[session_id] = run` has no guard. Two tabs or rapid reconnects for the same session overwrite each other. The first run's `finally` block calls `_runs.pop()`, which accidentally removes the *second* run. Cooperative stop signals break; zombie tasks remain. |
| 2 | **[FIXED] Resource leak: tool tasks never cancelled on generator teardown** | `navi/core/agent.py` | 842-892 | Each tool call runs in `asyncio.create_task()`. If the consumer (WebSocket handler) is cancelled, `GeneratorExit` breaks the `while True` loop at `yield item` but `tool_task` is never awaited or cancelled. It continues executing filesystem/SSH/shell commands in the background with no way to stop it. |
| 3 | **[FIXED] Empty LLM stream silently kills fallback** | `navi/llm/fallback.py` | 162-165, 208-210 | `stream()` and `stream_complete()` catch `StopAsyncIteration` from an empty generator and do `return`, ending the entire fallback chain instead of trying the next server. User sees an empty response with no error. |
| 4 | **[FIXED] Frontend race condition in session loading** | `webclient/src/stores/chat.js` | 34-58 | `loadSession()` guards against reloading the *same* ID, but has no guard against two *different* concurrent loads. If user switches A→B while `loadSession(A)` is in-flight, the late completion overwrites `currentId` back to A. UI snaps to the wrong session. |
| 5 | **[FIXED] Frontend crash on non-string tool result** | `webclient/src/components/messages/ContentCard.vue` | 100-111 | `metadata` computed property calls `.match()` on `props.tool.result ?? ''`. If `result` is an object or `null`, `.match()` throws `TypeError` and crashes the component. |
| 6 | **[FIXED] Frontend crash on malformed image data** | `webclient/src/stores/chat.js` | 393 | `buildMessageList` calls `b.startsWith('data:')` on every element of `m.images`. If backend sends `null` or a non-string, the history builder crashes. |

---

## High — Performance / Reliability / UX Breakage

| # | Problem | File | Line | Explanation |
|---|---------|------|------|-------------|
| 7 | **[FIXED] Unbounded WebSocket replay buffer** | `navi/api/websocket.py` | 44-45 | `_AgentRun.events` is a plain `list[dict]` with no size limit. Large tool results (e.g. 5 MB terminal output) accumulate. Every reconnect replays the entire buffer, causing memory and bandwidth spikes. |
| 8 | **[REMOVED] SQLite opens a new connection per operation** | `navi/core/sqlite_session_store.py` | 68, 79, 89, 101, 111, 121, 129 | SQLite support was removed entirely. PostgreSQL is now the only supported database. `aiosqlite` dependency removed, `db_path` config removed. |
| 9 | **[FIXED] Memory extraction task storm** | `navi/api/routes/sessions.py` | 50 | Every `POST /sessions` spawns a fire-and-forget `asyncio.create_task(_process_stale_sessions(...))`. No deduplication or rate-limiting. Rapid session creation launches many concurrent LLM calls, overwhelming the backend. |
| 10 | **[FIXED] Permanent blacklisting with no recovery** | `navi/llm/fallback.py` | 38-39 | `_dead_servers` and `_dead_models` are module-level sets. A transient network blip permanently blacklists the server until Python process restart. |
| 11 | **[FIXED] Subagent exception isolation missing** | `navi/core/agent.py` | 512 | Inside `run_ephemeral()`, tools execute with bare `await tool.execute()`. No `_run_with_sentinel` wrapper. If a tool crashes, the subagent dies and all partial progress is lost. |
| 12 | **[FIXED] No size validation on inline images** | `navi/api/websocket.py` | 297-304 | `raw_images` is taken directly from client JSON with no length or size checks. A client can send massive base64 payloads, causing memory exhaustion. |
| 13 | **[FIXED] Orphaned files when DB insert fails** | `navi/content_store.py` | 114-129 | In `publish()`, `shutil.copy2` runs first. If the subsequent DB `INSERT` fails (caught and logged), the copied file remains on disk with no metadata record, accumulating orphaned files in `navi/content/`. |
| 14 | **[FIXED] Frontend: broken streaming auto-scroll** | `webclient/src/components/chat/MessageList.vue` | 76-89 | Watchers on `chat.streamingMsg?.text` never fire during streaming because `streamingMsg` is a `shallowRef`. Vue does not track deep mutations, so the chat list does not auto-scroll as new deltas arrive. |
| 15 | **[FIXED] Frontend: mixed-content WebSocket on HTTPS** | `webclient/src/composables/useWebSocket.js` | 4-6 | WS URL is hardcoded to `ws://${location.host}`. When served over HTTPS, browsers block `ws://` as mixed content. Must switch to `wss://` when `location.protocol === 'https:'`. |
| 16 | **[FIXED] Frontend: duplicate message sends from rapid clicks** | `webclient/src/components/chat/InputBar.vue` | 141-160 | `handleSend` has no debounce or in-flight guard. `canSend` only checks `!chat.streaming`, which is not set until `stream_start`. Rapid clicks can queue multiple identical messages. |
| 17 | **[FIXED] Frontend: unhandled promise rejections across API layer** | `webclient/src/api/index.js` and callers | — | `request()` and `uploadFile()` throw on errors, but almost no callers wrap their awaits in `try/catch`. Failures become unhandled rejections and leave the UI in a broken silent state. |
| 18 | **SSH connections not closed on shutdown** | `navi/tools/ssh_exec.py` | 54-90 | `_PoolEntry` connections hang until TTL or process kill. No graceful cleanup on server shutdown. |

---

## Medium — Architectural / Edge Cases / Maintainability

| # | Problem | File | Line | Explanation |
|---|---------|------|------|-------------|
| 19 | **[FIXED] N+1 UPDATE queries in embedding backfill** | `navi/memory/store.py` | 170-176 | `backfill_embeddings()` loops over rows, issuing one `UPDATE` per row. Should batch via `executemany` or multi-row `UPDATE`. |
| 20 | **[FIXED] Unindexed ILIKE fallback search** | `navi/memory/store.py` | 282-314 | Text fallback constructs dynamic SQL with `ILIKE` on `category`, `key`, `value`. No indexes exist on these columns individually. Full table scan on large tables. |
| 21 | **[FIXED] Context list rebuilt from scratch every iteration** | `navi/core/agent.py` | ~683 | `_build_context()` creates a brand-new list by filtering and prepending system messages on every tool-calling iteration. O(n) per iteration, generating significant garbage. |
| 22 | **[FIXED] Unbounded `planning_logs` growth** | `navi/core/agent.py` | 651 | `session.planning_logs.append(_ev.log)` with no size limit or rotation. Long sessions with planning enabled produce huge JSON in DB, slowing `save()`. |
| 23 | **[FIXED] Background cleanup task unreferenced** | `navi/main.py` | 72 | `asyncio.create_task(cleanup_loop(...))` — the returned `Task` is not stored. In Python < 3.11, unhandled exceptions may be silently discarded. |
| 24 | **[FIXED] BaseException catches SystemExit** | `navi/core/agent.py` | 847-848 | `_run_with_sentinel` catches `BaseException`, which includes `SystemExit`. Should catch only `Exception`. |
| 25 | **[FIXED] SystemExit swallowed in agent loop** | `navi/core/agent.py` | 871 | `isinstance(r, BaseException)` includes `SystemExit`. It should propagate to allow graceful server shutdown. |
| 26 | **[FIXED] Hardcoded context output reserve** | `navi/core/agent.py` | 1521 | `output_reserve = 2048` tokens is not configurable per model. May falsely reject valid contexts for small models. |
| 27 | **[FIXED] Vector literal NaN/Infinity vulnerability** | `navi/memory/store.py` | 73-75 | `_vector_to_str` converts floats naively. A misbehaving embedding model returning `NaN` or `Infinity` creates invalid PostgreSQL vector syntax. |
| 28 | **Tight coupling to todo module internals** | `navi/core/agent.py` | 176-224 | `_todo_status_snapshot` directly reads `navi.tools.todo._plans`, a module-level dict. Hard to test and refactor. |
| 29 | **ContextVar values not reset in subagent path** | `navi/core/agent.py` | 350-351 | `run_ephemeral()` sets `_sid_var` and `_model_var` but never resets them. Background tasks spawned by the subagent may inherit stale values. |
| 30 | **[FIXED] Replay/reattach may miss post-finish events** | `navi/api/websocket.py` | 274-277 | If a client reconnects after a run finishes but before disconnect is fully processed, it gets `session_sync`. No push event indicates new messages were persisted while disconnected. |
| 31 | **[FIXED] Frontend: singleton confirm-dialog promise leak** | `webclient/src/composables/useConfirm.js` | 5-30 | Module-level `_resolve`. If `confirm()` is called twice before the first resolves, the second overwrites `_resolve`, causing the first promise to hang forever. |
| 32 | **[FIXED] Frontend: body overflow leak on lightbox unmount** | `webclient/src/components/ui/ImageLightbox.vue` | 26-28 | `watch(src)` sets `document.body.style.overflow = 'hidden'`. If component unmounts while open, the side effect is never reverted. Page stays permanently unscrollable. |
| 33 | **[FIXED] Frontend: DOM leak on fallback copy failure** | `webclient/src/composables/useCopy.js` | 10-17 | Fallback copy appends a `<textarea>` to body but lacks `try/finally`. If `document.execCommand('copy')` throws, the textarea remains in DOM. |
| 34 | **[FIXED] Frontend: reconnect race in WebSocket send()** | `webclient/src/composables/useWebSocket.js` | 52-64 | Multiple `send()` calls while `ws.readyState === CLOSING` each trigger `_connect()`, creating redundant sockets that are immediately destroyed. |

---

## Low — Code Smell / Minor

| # | Problem | File | Line | Explanation |
|---|---------|------|------|-------------|
| 35 | **No tests** | Entire project | — | Neither backend nor frontend has unit or integration tests. Refactoring is risky. |
| 36 | **[FIXED] Import-time side effects in deps** | `navi/api/deps.py` | 37, 88, 89 | `_memory_store`, `_session_store`, `_workers` instantiated at import time. SQLite table creation runs synchronously. Blocks health checks on lock failure. |
| 37 | **Hardcoded `old_webclient` static mount** | `navi/main.py` | 36-38 | `/static/` mounted from `old_webclient`. If renamed/removed, middleware fails. |
| 38 | **[FIXED] `__import__` anti-patterns** | `navi/content_store.py:188`, `navi/main.py:16-19` | 188, 16-19 | Inline `__import__` instead of direct imports. Hard to read and static analysis misses them. |
| 39 | **[FIXED] Token count `0` coerced to `None`** | `navi/llm/ollama.py` | 137-138 | `or None` converts legitimate `0` token count to `None`. Metrics lose precision. |
| 40 | **TODO: semantic deduplication** | `navi/memory/extractor.py` | 170 | Memory facts accumulate duplicates. Vector search + similarity threshold needed before upsert. |
| 41 | **Frontend: global `marked` singleton mutation** | `webclient/src/composables/useMarkdown.js` | 5-16 | `marked.setOptions()` and `marked.use()` mutate the global singleton. Conflicts with any other module wanting different options. |
| 42 | **Frontend: profile selector does not change active session profile** | `webclient/src/components/ui/AppSidebar.vue` | 16 | `@change` updates `profilesStore.selectedProfileId` directly but does not change the active session's profile. User thinks they switched but they didn't. |
| 43 | **[FIXED] Frontend: stale `streamingMsg` after reconnect reload** | `webclient/src/stores/chat.js` | 74-86 | `reloadSession` rebuilds `messages.value` but does not clear `streamingMsg.value`. Subsequent deltas mutate an orphaned object not in the rendered array. |
| 44 | **[FIXED] Frontend: incomplete HTML escaping in user messages** | `webclient/src/components/messages/UserMessage.vue` | 48-54 | `escapeHtml` encodes `& < > "` but not `'`. Defense-in-depth gap if ever injected into single-quoted attributes. |
| 45 | **Frontend: no message virtualization** | `webclient/src/components/chat/MessageList.vue` | 7-13 | All messages rendered in DOM. Long sessions degrade scroll/render performance. `vue-virtual-scroller` is already in the project but unused here. |
| 46 | **Frontend: fragile content-type detection from URL** | `webclient/src/components/messages/ContentCard.vue` | 120 | `url.split('.').pop()` does not strip query strings, so `image.png?size=large` fails lookup. |
| 47 | **Frontend: unused dependency** | `webclient/package.json` | 12 | `@tanstack/vue-virtual` listed but never imported; `vue-virtual-scroller` is used instead. |
| 48 | **[FIXED] Frontend: `findLast` compatibility risk** | `webclient/src/stores/chat.js` | 186-189, 257 | `Array.prototype.findLast` requires ES2023. Fails on older browsers unless polyfilled. |
| 49 | **[FIXED] Frontend: fragile table-wrapper regex** | `webclient/src/composables/useMarkdown.js` | 121 | `html.replace(/<table>/g, ...)` does not match `<table class="foo">`, so those tables miss the scrollable wrapper. |
| 50 | **[FIXED] Frontend: inefficient copy-button re-attachment** | `webclient/src/components/messages/AssistantMessage.vue` | 126 | `watch(renderedText)` calls `attachCopyButtons` on every streaming delta, repeatedly querying DOM even though buttons haven't changed. |
| 51 | **Frontend: imprecise relative-time updates** | `webclient/src/composables/useTime.js` | 36 | 30-second interval means "just now" can be stale for up to 30s before jumping. |
| 52 | **Frontend: stream message ID collision risk** | `webclient/src/stores/chat.js` | 148 | `id: 'stream_${Date.now()}'` could collide within the same millisecond on rapid reconnect. |
| 53 | **[FIXED] Frontend: flawed meta-row visibility logic** | `webclient/src/components/messages/AssistantMessage.vue` | 102-104 | `hasMeta` uses `|| props.msg.tool_call_count`, so `0` is falsy and may incorrectly hide the meta row. |
| 54 | **[FIXED] Dead `stream()` method on LLM backends** | `navi/llm/ollama.py:149`, `navi/llm/fallback.py:168` | `OllamaBackend.stream()` and `FallbackOllamaBackend.stream()` existed in code but were never called by `agent.py` or `messages.py`. Only `stream_complete()` was used. Removed from all backends and the base `LLMBackend` interface. |

---

## Security (Deferred — address before multi-user/public deployment)

| # | Problem | File | Line | Explanation |
|---|---------|------|------|-------------|
| S1 | **Arbitrary code execution (`code_exec`)** | `navi/tools/code_exec.py` | 54 | `asyncio.create_subprocess_exec(sys.executable, script_path)` with no sandbox (no seccomp, chroot, separate user). LLM-generated code runs with full server privileges. |
| S2 | **Full shell access by default (`terminal`)** | `navi/tools/terminal.py` | 66; `navi/config.py` | `TERMINAL_ALLOWED_COMMANDS=*` default. `create_subprocess_shell` allows any command including `rm -rf`, `curl \| sh`. |
| S3 | **Self-modifying code (`write_tool`)** | `navi/tools/write_tool.py` | 84 | LLM writes arbitrary `.py` to `tools/` and immediately imports it. Prompt injection → persistent backdoor without server restart. |
| S4 | **Path traversal in `content_publish`** | `navi/content_store.py` | 97-99 | `dest = dest_dir / dest_name` where `dest_name` is unsanitized user input. `../../etc/passwd` escapes the content directory. |
| S5 | **XSS via `v-html` in markdown rendering** | `webclient/src/composables/useMarkdown.js` | 119; `AssistantMessage.vue` | `renderMarkdown` returns raw HTML from `marked.parse()`. `v-html` in AssistantMessage, SummaryCard, CompressionNotice, ToolCard renders it unsanitized. |
| S6 | **Iframe sandbox bypass** | `webclient/src/components/messages/ContentCard.vue` | 26-41 | `sandbox="allow-scripts allow-same-origin"` on same-origin iframe removes all restrictions. Framed content can access `localStorage`, cookies, `window.parent`. |
| S7 | **Symlink traversal in `filesystem`** | `navi/tools/filesystem.py` | 71-88 | `Path.resolve()` follows symlinks. If `FS_ALLOWED_PATHS=/home/user` and a symlink points to `/etc`, `read` accesses `/etc/shadow`. |
| S8 | **SSH MITM by default** | `navi/tools/ssh_exec.py` | 254, 271 | `known_hosts="none"` disables host key verification. Vulnerable to man-in-the-middle attacks. |
| S9 | **Weak content UUID (8 chars)** | `navi/content_store.py` | 93 | `str(uuid.uuid4())[:8]` — only 4 billion combinations. `/content` is open without auth. Brute-force guessable. |
| S10 | **Passwords in plaintext in SSH pool** | `navi/tools/ssh_exec.py` | 48-51 | `_PoolEntry.connect_kwargs` stores passwords/keys. No scrubbing in logs or memory. |
| S11 | **No image size validation on WS** | `navi/api/websocket.py` | 297-304 | `raw_images` taken directly from client JSON without length/size checks. Malicious client can OOM the server. |
| S12 | **Indirect recursion in `spawn_agent`** | `navi/tools/spawn_agent.py` | 128 | `exclude_tools=["spawn_agent"]` blocks direct call, but sub-agent can call `write_tool` → new tool → `spawn_agent`. |
| S13 | **No `working_dir` validation in `terminal`** | `navi/tools/terminal.py` | 70 | `working_dir` passed directly to `cwd`. Any directory accessible if unrestricted mode. |
