Conversation
- Handle TOCTOU race in registration by catching Prisma P2002 errors - Add GET to CORS allowed methods so non-registration endpoints work - Set mode 0o600 on persisted license key file - Create new license for re-registering customers with no active one - Remove dead error prop from UpgradePrompt - Remove process.env mutation in activateLicenseKey - Fix fallback registration URL port from 3002 to 3001 Co-Authored-By: Claude <noreply@anthropic.com>
|
@cursor review |
|
@cursor review |
Code reviewFound 1 issue:
monitor/apps/web/src/api/registration.ts Lines 1 to 2 in 2e8b23d 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
|
Thank you for your contribution! Before we can merge this PR, you need to sign our Contributor License Agreement. To sign, please comment below with:
I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
Co-authored-by: Kristiyan Ivanov <k.ivanow@gmail.com>
| } catch (error) { | ||
| this.logger.error(`Failed to send email to ${to}:`, error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Error double-logged on Resend API failure
Low Severity
When the Resend API returns a non-OK response, the error is logged at line 66 with response details, then the thrown Error is immediately caught by the outer catch block at line 72 which logs it again. This produces redundant log entries for every failed email delivery attempt.
Reviewed by Cursor Bugbot for commit 325259c. Configure here.
There was a problem hiding this comment.
Some extra logging is fine
* feature: integrated CLI panel for running Valkey commands (#91) Adds a WebSocket-based CLI panel to the Monitor UI that lets users run Valkey/Redis commands directly from the browser. The CLI operates in two modes: safe (read-only allowlist, default) and unsafe (all commands, via BETTERDB_UNSAFE_CLI=true env var). Backend: - WebSocket gateway at /cli/ws with dedicated Valkey client per connection - Command parser ported from VS Code extension (quoted strings, escapes) - Two-tier command filtering: safe mode allowlist + always-blocked list - Response formatting matching valkey-cli output style - 30s command timeout, 512KB response truncation, 1MiB WS payload limit - Token-bucket rate limiter (50 cmd/s per connection) - Cloud-mode session cookie auth on WS upgrade Frontend: - Collapsible bottom panel using shadcn Collapsible, ScrollArea, Tooltip - Command history with up/down arrow navigation (ref-based, no re-renders) - Auto-reconnecting WebSocket hook with exponential backoff - Built-in commands: help, clear, history, exit - Ctrl+` keyboard shortcut to toggle panel Closes #92, closes #93, closes #94 * fix: address PR review findings from Bugbot - Replace single pendingCommandRef with a queue to support rapid commands - Reference-count CLI Valkey clients so multi-tab usage doesn't kill shared connections on single WS disconnect - Remove non-functional ScrollArea wrapper (inner div handled overflow) * fix: address second round of Bugbot findings - Set isConnected=false immediately on connectionId change reconnect - Prevent duplicate Valkey client creation from concurrent WS messages using an inflight connection promise map * fix: serialize command execution and fix cookie parser - Chain command execution per WS client to guarantee FIFO response order, preventing out-of-order response matching - Fix cookie parser truncating JWT values containing '=' signs * fix: remove custom auth from CLI gateway, use existing CloudAuthMiddleware The project's CloudAuthMiddleware already runs on all HTTP requests including WebSocket upgrades. Removed duplicated JWT validation and cookie parsing from CliGateway, and removed /cli/ws from the middleware's whitelist so it properly checks auth in cloud mode. * fix: pass TLS config to CLI Valkey client, remove unused scroll-area - CLI Valkey client now respects the connection's TLS setting - Deleted unused scroll-area.tsx (ScrollArea was removed in earlier fix) * fix: use ConnectionRegistry.createAdapter for CLI connections Instead of hand-rolling Valkey client creation, the CLI now uses the registry's createAdapter() factory with connectionName 'BetterDB-CLI'. This reuses existing connection config handling (TLS, credentials, dbIndex) and follows the project's adapter pattern. Made createAdapter() public and extended UnifiedDatabaseAdapterConfig to accept optional connectionName, tls, and dbIndex. * fix: CollapsibleTrigger now toggles the CLI panel both ways The onOpenChange handler was only calling onClose, so clicking the trigger bar couldn't open the panel. Now passes onToggle to Collapsible. * chore: consolidate CLI tests with it.each fixtures Replaced repetitive individual test cases with it.each tables in both command-parser and cli.service specs. Same coverage, less boilerplate. * chore: extract command allowlists to packages/shared Moved ALLOWED_COMMANDS, ALLOWED_SUBCOMMANDS, BLOCKED_COMMANDS, and BLOCKED_SUBCOMMANDS to packages/shared/src/types/command-safety.ts. Both the cloud agent (packages/agent) and CLI service now import from the same source, eliminating drift risk. CLI safe mode uses the exact same allowlist as the agent. * chore: add description for BETTERDB_UNSAFE_CLI, refactor constants in cli.service Clarifies the purpose of the `BETTERDB_UNSAFE_CLI` variable with a schema description and reorganizes `MAX_RESPONSE_SIZE` placement for readability. * fix: revert tls/dbIndex additions to UnifiedDatabaseAdapterConfig These are out of scope for the CLI feature. The only change needed was adding the optional connectionName parameter. TLS and dbIndex support for the adapter is a separate concern. * fix: remove unsafeCliEnabled from health response Not used by the frontend and leaks security config to public endpoint. Removed field from HealthResponse type, ConfigService injection from HealthService. * fix: use NestJS Logger instead of console for CLI startup messages * fix: replace let with const for agentGateway using IIFE * chore: apply linter formatting * fix: remove redundant connection name badge and status text from CLI header Connection info is already visible in the sidebar, and the green dot on the trigger bar shows connection status. * fix: remove trash and close buttons from CLI panel header Redundant — clear is available via 'clear' command / Ctrl+L, and close via trigger bar click / Ctrl+`. * feature: resizable CLI panel via drag handle Added a drag handle at the top of the panel. Users can drag to resize between 200px and 70% of viewport height. Default is 30vh. * fix: add visible grip indicator to CLI resize handle * fix: remove quotes from string responses in CLI output valkey-cli uses quotes to distinguish types, but in the web UI it looks wrong. Strings are now returned as-is. * chore: apply linter formatting * fix: auto-scroll at max entries, remove unused resizable component - Changed auto-scroll dependency from entries.length to entries ref so it fires even when length stays at MAX_ENTRIES (500) - Deleted unused resizable.tsx (custom drag handle is used instead) * fix: remove unused success styling in CLI panel * fix: remove unused react-resizable-panels dependency * chore: add example for BETTERDB_UNSAFE_CLI in `.env.example` * fix: compare BETTERDB_UNSAFE_CLI against string, not boolean ConfigService returns raw env strings, so comparing against boolean true always evaluated to false. Use string comparison consistent with other boolean env vars in configuration.ts. * fix: update unsafe mode test mock to return string value The ConfigService mock was returning boolean true, but the service now compares against string 'true' to match raw env var behavior. * feature: unify CLI command safety logic across agent and API - Move checkBlocked/checkSafeMode functions to @betterdb/shared - CLI service uses connectionRegistry.get() instead of creating adapters - Add call() to DatabasePort interface for generic command execution - Agent supports BETTERDB_UNSAFE_CLI with lazy CLI client connection - CLI commands tagged with cli flag in WebSocket protocol * feature: add dedicated CLI client to unified adapter Use a separate lazily-created Valkey connection for CLI commands, isolating interactive traffic from monitoring queries.
- Handle TOCTOU race in registration by catching Prisma P2002 errors - Add GET to CORS allowed methods so non-registration endpoints work - Set mode 0o600 on persisted license key file - Create new license for re-registering customers with no active one - Remove dead error prop from UpgradePrompt - Remove process.env mutation in activateLicenseKey - Fix fallback registration URL port from 3002 to 3001 Co-Authored-By: Claude <noreply@anthropic.com>
Resolve conflicts keeping: - TooltipProvider wrapper and error prop pattern from master - Validate-before-persist activation flow from branch - ServiceUnavailableException error handling from branch - Masked license key logging from master - CORS and registration URL fixes from branch Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a5848d. Configure here.
| await this.cliClient.connect(); | ||
| this.logger.log('CLI client connected'); | ||
| return this.cliClient; | ||
| } |
There was a problem hiding this comment.
Race condition in lazy CLI client initialization
Medium Severity
getCliClient() has a race condition during lazy initialization. The first caller sets this.cliClient synchronously via createValkeyClient() then awaits connect(). A second concurrent caller sees this.cliClient is non-null and returns it immediately — before connect() has resolved. Since the client is created with enableOfflineQueue: false, commands sent on the not-yet-connected client will fail. The same pattern exists in Agent.getCliExecutor().
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8a5848d. Configure here.
There was a problem hiding this comment.
not from this branch - caused by the updated with master
jamby77
left a comment
There was a problem hiding this comment.
Other than that localhost:3001 (API url) does not need to be in CORS, everything else seems fine


Testing steps:
Here's the full local testing setup:
Start Postgres (test instance on port 5433)
docker compose -f docker-compose.test.yml up -d postgres
Start the entitlement service (port 3002)
cd proprietary/entitlement
Create .env with:
ENTITLEMENT_DATABASE_URL="postgresql://betterdb:devpassword@localhost:5433/betterdb"
RESEND_API_KEY=re_your_key_here
Run migrations (first time only)
ENTITLEMENT_DATABASE_URL="postgresql://betterdb:devpassword@localhost:5433/betterdb" pnpm prisma:migrate
pnpm prisma:generate
PORT=3002 pnpm dev
From repo root, with these in .env or inline:
VITE_REGISTRATION_URL=http://localhost:3002/v1/registrations
ENTITLEMENT_URL=http://localhost:3002/v1/entitlements
Unset any existing license key
unset BETTERDB_LICENSE_KEY
ENTITLEMENT_URL=http://localhost:3002/v1/entitlements pnpm dev
Open http://localhost:5173 → Settings → License
Register with an email
Check entitlement service console for the license key (if no Resend key) or check email
Paste the key in "Already have a license key?" → Activate
Should upgrade to Enterprise tier
List what's there
docker exec betterdb-test-postgres psql -U betterdb -d betterdb -c "SELECT id, key FROM licenses;"
Delete a specific registration
docker exec betterdb-test-postgres psql -U betterdb -d betterdb -c
"DELETE FROM licenses WHERE customer_id = ''; DELETE FROM customers WHERE email = '';"
Note
High Risk
Adds a new WebSocket-backed CLI that can execute database commands (including an opt-in unsafe mode) and introduces new registration/email + runtime license activation paths, expanding the system’s attack surface and affecting licensing state persistence.
Overview
Adds an integrated Valkey/Redis CLI panel backed by a new API WebSocket endpoint (
/cli/ws). The backend introducesCliGateway+CliServicewith command parsing, rate limiting, timeouts, output truncation/formatting, and shared safe/blocked command enforcement (withBETTERDB_UNSAFE_CLI=trueto allow writes), plus a dedicatedBetterDB-CLIValkey client path in the database adapters.Implements a free registration + activation flow for licensing. The web app adds a new Settings License section that can register an email (via
VITE_REGISTRATION_URL) and activate a license key via a newPOST /license/activateendpoint; the API now persists activated keys to disk and hardens error handling/message extraction, while the entitlement service adds a/v1/registrationsendpoint that creates/resends enterprise licenses and sends emails via Resend (plus CORS/env updates and cloud tenant-based entitlement resolution viatenantId).Reviewed by Cursor Bugbot for commit 9e89017. Bugbot is set up for automated code reviews on this repo. Configure here.