Void-Homelab/docs/code-review-2026-06-01.md

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.