# Void 2.0 — Code Review & Optimisation Notes (2026-06-01, alpha.6)

Full read-through of `lib/` (~4.2k LOC), the public SPA, and the Python workers.
Overall the codebase is in good shape: ~1:1 test-to-code ratio, consistent repo +
audit pattern, clean tool registry, disciplined safe-DOM and SSRF handling. Items
below are improvements, ordered by value. Severity ≠ urgency — most are "before
scale / before more agents", not "broken now".

> **Resolved 2026-06-02:** #1 (pool error handler), #2 (upsert-arm guard), #3
> (`verifyToken` O(1) selector+verifier), and #7 (`context` column allow-list)
> are **done** — see the security sweep doc + their tests. Remaining open: #4
> (FTS index alignment — needs a prod `EXPLAIN`), #5 (dedupe bearer parsing), #6
> (doc/code symbol drift).

## Correctness / robustness

1. **`pool.js` has no error handler or statement timeout.** `lib/db/pool.js`
   creates a `pg.Pool` with no `pool.on('error', …)`. An error on an *idle*
   pooled client (e.g. DB restart, replication failover on the .215 cluster)
   emits an `error` event with no listener → **process crash**. Add a logging
   error handler. Also consider `statement_timeout` / `query_timeout` so a
   pathological query can't pin a connection indefinitely (the pool is only
   `max: 10`).

2. **`applyPendingChange`'s `upsert` arm assumes a refs-shaped repo.** After
   today's fix, `case 'upsert'` calls `repo.upsertByExternal(...)`. Only
   `refsRepo` has that method; the action only originates from the refs route so
   it's safe today, but it's an implicit coupling. Cheap guard: assert
   `typeof repo.upsertByExternal === 'function'` and throw a clear
   `ValidationError` otherwise, so a future entity emitting `upsert` fails loud,
   not with a bare `TypeError`.

## Performance / scale

3. **`verifyToken` is O(n·bcrypt) per request.** (Also in the security sweep.)
   Loads every non-revoked token and bcrypt-compares each. Fine at 1–2 agents,
   quadratic-feeling as agents grow. Selector+verifier split makes it O(1). This
   is the single highest-value optimisation once Yerin/others get their own tokens.

4. **Hybrid-search FTS uses inline `to_tsvector(...)` expressions.** `search.js`
   computes `to_tsvector('english', title || …)` at query time per branch. This
   only avoids a seq scan if a **GIN index on the identical expression** exists
   for each table. Verify the expression indexes match these exact expressions
   (incl. the `coalesce(...)` shapes); a drift means silent seq scans. Worth a
   `EXPLAIN` check on prod-sized data.

## Cleanliness / maintainability

5. **`owner.js` and `agent_auth.js` duplicate bearer parsing + owner check.**
   Both split `Authorization`, both compare `OWNER_TOKEN`. Now that both use
   `timingSafeStrEqual`, factor the "parse bearer + is-owner?" step into one
   helper to keep the two in lockstep.

6. **Doc/code name drift.** Completion docs reference `divertToPending` /
   `applyPendingChange` — those are now the real names (good), but earlier notes
   also referenced a `n()` export. Keep `docs/plan-*-complete.md` symbol names in
   sync with the code; stale symbol names cost a grep on every revisit.

7. **`context` tool `SELECT *`.** Project an explicit column list (see security
   sweep LOW) — also makes the tool's output contract stable for the UI.

## Things that are good (keep doing)
- Tool registry (`createRegistry`) is a clean extension point — the new
  `securityRegistry` reused it with zero changes.
- Audit emission + recursive redaction is consistent across repos.
- SSRF `safeFetch` is genuinely well built (DNS-pin TOCTOU defeat + per-hop
  re-validation).
- Test isolation on `void_test` with a prod-guard in `tests/helpers/setup.js`.