Feature/iovalkey wrapper

[KIvanow]

Summary

Adds client-side command capture: a new @betterdb/iovalkey-capture package that wraps iovalkey and tees commands from instrumented applications to Monitor, plus the backend session/ingest pipeline and UI to control capture windows. Unlike server-side MONITOR, this works on managed instances where the MONITOR command is disabled, and captures only the traffic of opted-in clients with bounded overhead.

Changes

• @betterdb/iovalkey-capture package — drop-in CaptureValkey extends Redis wrapper. Intercepts via a sendCommand override (covers pipelines/multi), buffers with a bounded cap, batches POSTs to Monitor, and polls for active capture windows. Capture failures never propagate to the caller; stats() exposes captured/dropped/failed counts.
  • Backend (apps/api/src/command-capture/) — session lifecycle (start/stop/expire with optional command cap), wrapper-facing window-poll and batch-ingest endpoints guarded by agent token + instance check, admin endpoints for the UI, and 3-day retention with throttled inline pruning.
  • Storage — command_capture_sessions / command_capture_records support across memory, SQLite, and Postgres adapters.
  • Expiry enforcement (defense in depth) — wrapper gates each command against an absolute expiresAt from the poll response; ingest rejects past a 5s grace window and enforces the command cap server-side.
  • Web UI — Monitor page renamed to "Monitor / Command Capture" with a Server (MONITOR) / Client Library toggle; new CommandCaptureControl for starting/stopping capture windows with duration presets, optional command cap, and live countdown/command count.

Checklist

• Unit / integration tests added
• Docs added / updated
• Roborev review passed — run roborev review --branch or /roborev-review-branch in Claude Code (internal)
• Competitive analysis done / discussed (internal)
• Blog post about it discussed (internal)

---

Note

Medium Risk
New ingest path stores full Redis command names/args (may include secrets) and relies on agent tokens that are not instance-scoped; bounded caps and retention limit blast radius but the feature touches auth and sensitive workload data.

Overview
Adds client-side command capture alongside existing server MONITOR capture: a new @betterdb/iovalkey-capture package (CaptureValkey subclasses iovalkey, tees commands via sendCommand, polls for active windows, and POSTs batched commands to Monitor without affecting caller Redis behavior).

The API gains CommandCaptureModule with session lifecycle (one active window per connection, optional command cap, lazy expiry/stop), wrapper routes under api/capture/instance/:instanceId (window + batch, AgentTokenGuard + registry check), and admin routes under api/command-capture for start/stop/status. Captured commands persist through new command_capture_sessions / command_capture_records storage APIs on memory, SQLite, and Postgres, with ~3-day retention and throttled prune on ingest.

Expiry and caps are enforced on both sides: the wrapper gates on expiresAt and stops at cap; ingest uses a 5s post-expiry grace, drops when no session or at cap, and updates commandCount.

The web app renames the Monitor area to Monitor / Command Capture, adds a Server (MONITOR) vs Client Library tab, and CommandCaptureControl to start/stop windows with duration presets, optional cap, and live countdown/command count. Shared types for command-capture sessions/records live in @betterdb/shared.

^{Reviewed by Cursor Bugbot for commit 990ba0f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes and found 3 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 990ba0f. Configure here.}

[cursor]

Duplicate api route prefix

High Severity

The capture controllers register under api/... while other Nest routes use paths like monitor and rely on setGlobalPrefix('api') in production. That yields /api/api/... in prod (breaking CaptureValkey calls to /api/capture/...) and /api/command-capture/... in dev while the web client calls /command-capture/... without the extra segment.

 

^{Reviewed by Cursor Bugbot for commit 990ba0f. Configure here.}

[cursor]

Ingest ignores command cap remainder

High Severity

ingestBatch only rejects when commandCount is already at the cap, then persists every command in the batch. A single large batch can push commandCount well above commandCap, contradicting server-side cap enforcement described in the PR.

 

^{Reviewed by Cursor Bugbot for commit 990ba0f. Configure here.}

[cursor]

Concurrent ingest loses command count

Medium Severity

Each ingest reads commandCount, inserts records, then writes commandCount + saved from that stale snapshot. Overlapping batch requests can overwrite each other’s increments, so totals and cap checks diverge from rows actually stored.

 

^{Reviewed by Cursor Bugbot for commit 990ba0f. Configure here.}

[jamby77]

Reviewed at 990ba0f6. Flagging up front that this is a draft, six weeks
stale, and 230 commits behind master with 5 conflicting files — so treat this
as notes for when it gets picked back up rather than a merge verdict.

The shape is good: the sendCommand override is the right interception point
(it covers pipelines and multi for free), the bounded buffer with a
never-propagate-to-caller policy is the correct trade for a client-side tee,
and enforcing expiry on both sides rather than trusting the wrapper is the
right instinct. Five things I'd want resolved before it leaves draft.

---

1. The wrapper captures the Valkey password in plaintext

CaptureValkey.ts:89 stringifies every argument of every command with no
denylist. iovalkey issues AUTH through sendCommand — event_handler.js
calls self.auth(self.condition.auth, …) in the connect handler, and auth
is a generated command method that lands in the same override this subclasses.

So any reconnect while a window is active captures AUTH <password> (or
AUTH <username> <password>) and ships it to Monitor, where
command_capture_records.args holds it for three days and it is removed only
by age. command-capture.controller.ts:107 allows windows up to 24h, which
makes a reconnect inside a window close to certain in production.

It is not only AUTH. SET session:abc <bearer-token> is captured the same
way, which is what makes a plain argument capture a much bigger promise than
it looks — the PR body's "may include secrets" is understating it, because one
of the secrets is the credential for the very database being monitored.

Minimum: deny AUTH and HELLO outright. Better: capture command name and
argument count by default, with argument capture opt-in per session and a
visible warning, since the common diagnostic questions (which commands, how
often, from which client) do not need values.

2. Nothing can read what gets captured

The storage port additions are save + prune only — there is no
getCommandCaptureRecords, no API route that reads records, and nothing in
the UI beyond commandCount. Grepping command_capture_records across the
branch turns up an INSERT, a DELETE, and two index definitions.

Commands land in a table, are counted, and are deleted three days later
without ever being visible. Everything else here — the wrapper, the ingest
path, the storage on three adapters, the control UI — exists to feed a read
path that is not in the PR. Plausible for a draft, but it is the deliverable,
so worth being explicit about whether it is deferred or missed.

3. The controller that starts a capture has no guard

command-capture.controller.ts:70-71 — CommandCaptureAdminController has no
@UseGuards, while the wrapper-facing controller directly above it at line 27
has AgentTokenGuard, and MonitorController gates its equivalent routes with
LicenseGuard. The docstring at line 68 says "Uses the same auth pattern as
the monitor controller"; it does not.

Most controllers in this app are unguarded, so this is not an outlier in
absolute terms. What makes it worth fixing is the asymmetry inside one file:
in cloud mode AgentTokenGuard rejects unauthenticated ingest, while
POST /api/command-capture/start — which begins recording every command and
argument for a connection — takes anyone. The more dangerous verb has the
weaker gate. LicenseGuard would also match how the MONITOR sibling feature
is gated.

4. The command cap can be overrun

command-capture.service.ts:180-182 reads active.commandCount and writes
active.commandCount + saved. Two batches ingesting concurrently — which is
the normal case, since the point of this feature is multiple instrumented
clients — both read N and both write N + their own count, losing one
increment. The cap is then checked against an undercount, so a capped window
overruns by however much was lost.

The wrapper's own windowCapturedCount does not compensate: it is per
connection, and the cap is per session across all wrappers.

Both adapters can do this atomically —
SET command_count = command_count + ? — which needs a patch shape that
expresses an increment rather than an absolute.

5. LIMIT is interpolated rather than parameterised

sqlite.adapter.ts:4220 and postgres.adapter.ts:4496 both build
LIMIT ${options.limit} by string interpolation while parameterising
everything else in the same query. Every other LIMIT in sqlite.adapter.ts
on master is LIMIT ? — this is a divergence from the file's own convention,
not the house style.

Only reachable with a literal 1 from the service today, so not exploitable
as written. Worth closing while the code is new rather than leaving a
${} in a SQL string for someone to widen later.

---

Smaller things, not worth blocking on:

• flush() drops the batch on any non-2xx or network error with no retry.
  Deliberate and surfaced through stats().failedFlushCount, but silent loss
  in a diagnostic tool is worth a line in the README so nobody reads a partial
  capture as a complete one.
• command.args.map(String) mangles Buffer arguments. Fine for text
  workloads, lossy for binary values, and there is no marker distinguishing
  the two after the fact.
• assertInstanceExists depends on registry.get() throwing rather than
  returning. True today; a silent authorisation bypass if that ever changes.
  An explicit check reads better than a comment explaining the implicit one.

Happy to look again once it is rebased — the conflicts alone will need a pass.