-
-
Notifications
You must be signed in to change notification settings - Fork 18
Backend agent refactoring #1466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the RocketAdmin agent from a NestJS-based application to a standalone CLI tool. The changes eliminate the NestJS framework dependency and replace the interactive CLI interface (readline-sync) with a modern alternative (inquirer), while improving the overall user experience with better visual feedback and structured command-line options.
Key Changes:
- Converted from NestJS server architecture to a lightweight standalone CLI application with WebSocket client
- Replaced synchronous readline-sync with asynchronous inquirer for interactive prompts
- Added Commander.js for CLI argument parsing and chalk/ora for enhanced terminal UI
- Introduced dedicated WebSocket client class for managing server connections
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Added inquirer, chalk, ora, commander dependencies and their sub-dependencies; removed NestJS-specific packages like @nestjs/platform-ws, readline-sync, and reflect-metadata |
| rocketadmin-agent/package.json | Updated dependencies, removed NestJS packages, added CLI tools (inquirer, chalk, ora, commander), changed build scripts from nest-cli to direct tsc, added bin entry point |
| rocketadmin-agent/src/main.ts | Complete rewrite from NestJS bootstrap to CLI application with Commander.js, added interactive setup wizard, improved connection testing with retry logic |
| rocketadmin-agent/src/websocket/websocket-client.ts | New WebSocket client implementation with reconnection logic, spinner feedback, graceful shutdown handling |
| rocketadmin-agent/src/helpers/cli/interactive-prompts.ts | New interactive prompts module using inquirer with validation, better UX with color-coded messages and visual feedback |
| rocketadmin-agent/src/helpers/cli/cli-questions.ts | Removed old readline-sync based CLI questions implementation |
| rocketadmin-agent/src/shared/config/config.ts | Updated to use async interactive prompts instead of sync CLI questions |
| rocketadmin-agent/src/app.{module,service,controller}.ts | Removed NestJS application files no longer needed |
| rocketadmin-agent/Dockerfile | Updated build and start commands to use direct node execution instead of nest-cli |
| backend/test/utils/register-user-and-return-user-info.ts | Added conditional SaaS registration handling |
| backend/test/ava-tests/saas-tests/connection-e2e.test.ts | Added debug logging and test assertions |
| backend/src/shared/config/config.service.ts | Increased database connection pool max from 2 to 10 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.ws?.send(JSON.stringify(e)); | ||
| } | ||
| } catch (e) { | ||
| console.error(chalk.red('Failed to process message:'), e); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message "Failed to process message:" doesn't provide enough context about what went wrong. Consider including information about the message structure or parsing failure to help with debugging. Additionally, the error is only logged but not sent back to the server, which might leave the server waiting for a response indefinitely.
| console.error(chalk.red('Failed to process message:'), e); | |
| // Log the error with more context | |
| console.error( | |
| chalk.red('Failed to process message:'), | |
| { | |
| rawData: data.toString(), | |
| error: e | |
| } | |
| ); | |
| // Try to extract resId if possible | |
| let resId: string | undefined = undefined; | |
| try { | |
| const parsed = JSON.parse(data.toString()); | |
| if (parsed && parsed.data && parsed.data.resId) { | |
| resId = parsed.data.resId; | |
| } | |
| } catch (_) { | |
| // Ignore, can't parse resId | |
| } | |
| // Send error response back to server | |
| const errorResponse = { | |
| operationType: OperationTypeEnum.dataFromAgent, | |
| error: { | |
| message: 'Failed to process message', | |
| details: e instanceof Error ? e.message : String(e), | |
| }, | |
| resId: resId, | |
| }; | |
| this.ws?.send(JSON.stringify(errorResponse)); |
| }; | ||
| this.ws?.send(JSON.stringify(responseData)); | ||
| } catch (e) { | ||
| this.ws?.send(JSON.stringify(e)); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling in the inner try-catch sends the raw error object to the server with JSON.stringify(e), which may not serialize properly and could expose sensitive stack traces or internal error details. Consider sending a structured error response with a safe error message instead.
| this.ws?.send(JSON.stringify(e)); | |
| const errorResponse = { | |
| operationType: OperationTypeEnum.dataFromAgent, | |
| error: { | |
| message: e instanceof Error ? e.message : 'An unexpected error occurred', | |
| }, | |
| resId: resId, | |
| }; | |
| this.ws?.send(JSON.stringify(errorResponse)); |
| .set('Accept', 'application/json'); | ||
|
|
||
| const foundOneRo = JSON.parse(findOneResponce.text); | ||
| console.log('🚀 ~ foundOneRo:', foundOneRo); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug console.log statements should not be committed to the codebase. This appears to be a debugging statement that was left in accidentally. Consider removing it or replacing it with proper test diagnostics if needed.
| console.log('🚀 ~ foundOneRo:', foundOneRo); |
| function getWebSocketUrl(cliOption?: string): string { | ||
| const defaultUrl = 'wss://ws.rocketadmin.com:443/'; | ||
| const wsUrl = cliOption || process.env.REMOTE_WEBSOCKET_ADDRESS || defaultUrl; | ||
| console.log(chalk.gray(`→ WebSocket URL: ${wsUrl}`)); | ||
| console.log(chalk.gray(` (env REMOTE_WEBSOCKET_ADDRESS: ${process.env.REMOTE_WEBSOCKET_ADDRESS || 'not set'})`)); | ||
| return wsUrl; | ||
| } | ||
|
|
||
| async function main(): Promise<void> { | ||
| program.parse(); | ||
| const options = program.opts(); | ||
| const wsUrl = getWebSocketUrl(options.wsUrl); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug log showing the environment variable status is always printed even when the default is used. Consider only logging this at a verbose/debug level or when explicitly requested, as it adds noise to normal operations.
| function getWebSocketUrl(cliOption?: string): string { | |
| const defaultUrl = 'wss://ws.rocketadmin.com:443/'; | |
| const wsUrl = cliOption || process.env.REMOTE_WEBSOCKET_ADDRESS || defaultUrl; | |
| console.log(chalk.gray(`→ WebSocket URL: ${wsUrl}`)); | |
| console.log(chalk.gray(` (env REMOTE_WEBSOCKET_ADDRESS: ${process.env.REMOTE_WEBSOCKET_ADDRESS || 'not set'})`)); | |
| return wsUrl; | |
| } | |
| async function main(): Promise<void> { | |
| program.parse(); | |
| const options = program.opts(); | |
| const wsUrl = getWebSocketUrl(options.wsUrl); | |
| function getWebSocketUrl(cliOption?: string, verbose?: boolean): string { | |
| const defaultUrl = 'wss://ws.rocketadmin.com:443/'; | |
| const wsUrl = cliOption || process.env.REMOTE_WEBSOCKET_ADDRESS || defaultUrl; | |
| console.log(chalk.gray(`→ WebSocket URL: ${wsUrl}`)); | |
| if (verbose) { | |
| console.log(chalk.gray(` (env REMOTE_WEBSOCKET_ADDRESS: ${process.env.REMOTE_WEBSOCKET_ADDRESS || 'not set'})`)); | |
| } | |
| return wsUrl; | |
| } | |
| async function main(): Promise<void> { | |
| program.parse(); | |
| const options = program.opts(); | |
| const wsUrl = getWebSocketUrl(options.wsUrl, options.verbose); |
| // eslint-disable-next-line security/detect-possible-timing-attacks | ||
| if (input !== password) { |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing passwords using strict equality operator can be vulnerable to timing attacks. The eslint-disable comment acknowledges this but doesn't address the security concern. For password comparison, consider using a constant-time comparison function to prevent timing-based attacks that could reveal information about the password.
| static displayConfigSummary(config: ICLIConnectionCredentials): void { | ||
| console.log(''); | ||
| console.log(chalk.cyan('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━')); | ||
| console.log(chalk.white.bold(' Connection Configuration')); | ||
| console.log(chalk.cyan('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━')); | ||
| console.log(` ${chalk.gray('Database Type:')} ${chalk.white(config.type)}`); | ||
| console.log(` ${chalk.gray('Host:')} ${chalk.white(config.host)}`); | ||
| console.log(` ${chalk.gray('Port:')} ${chalk.white(config.port)}`); | ||
| if (config.username) { | ||
| console.log(` ${chalk.gray('Username:')} ${chalk.white(config.username)}`); | ||
| } | ||
| if (config.database) { | ||
| console.log(` ${chalk.gray('Database:')} ${chalk.white(config.database)}`); | ||
| } | ||
| if (config.schema) { | ||
| console.log(` ${chalk.gray('Schema:')} ${chalk.white(config.schema)}`); | ||
| } | ||
| console.log(` ${chalk.gray('SSL:')} ${chalk.white(config.ssl ? 'Enabled' : 'Disabled')}`); | ||
| console.log(chalk.cyan('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━')); | ||
| console.log(''); | ||
| } |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The displayConfigSummary function displays sensitive configuration details including host, port, username, and database name in plain text to the console. While the password is not displayed, this information could still be valuable to an attacker. Consider warning users that this information is being displayed, or add an option to suppress this output in production environments where console output might be logged.
| const { | ||
| data: { resId }, | ||
| } = messageData; | ||
|
|
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message parsing assumes a specific structure with nested data.resId, but there's no validation that this structure exists before accessing it. If the message format is different, this will throw an error that gets caught in the outer catch block at line 96, losing the original message data. Consider validating the message structure before attempting to access nested properties.
| const { | |
| data: { resId }, | |
| } = messageData; | |
| // Validate messageData structure before destructuring | |
| if ( | |
| !messageData || | |
| typeof messageData !== 'object' || | |
| !('data' in messageData) || | |
| typeof messageData.data !== 'object' || | |
| messageData.data === null || | |
| !('resId' in messageData.data) | |
| ) { | |
| console.error( | |
| chalk.red('Received message with invalid structure:'), | |
| messageData | |
| ); | |
| return; | |
| } | |
| const { resId } = messageData.data; |
| await Config.setConnectionConfig(interactiveConfig, true); | ||
| await startAgent(interactiveConfig, wsUrl); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ERR_USE_AFTER_CLOSE') { |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error code check ERR_USE_AFTER_CLOSE is very specific to one type of cancellation scenario. There may be other ways users can cancel the setup (like SIGINT during prompt) that won't match this error code and will fall through to the generic error handler. Consider checking for more cancellation patterns or handling this more generically.
| if ((error as NodeJS.ErrnoException).code === 'ERR_USE_AFTER_CLOSE') { | |
| const err = error as any; | |
| if ( | |
| err?.code === 'ERR_USE_AFTER_CLOSE' || | |
| err?.code === 'ABORT_ERR' || | |
| err?.name === 'AbortError' || | |
| err?.name === 'PromptAbortedError' || | |
| (typeof err?.message === 'string' && /cancelled|canceled/i.test(err.message)) || | |
| err?.signal === 'SIGINT' | |
| ) { |
| const { confirmPassword: _confirmPassword } = await inquirer.prompt([ | ||
| { | ||
| type: 'password', | ||
| name: 'confirmPassword', | ||
| message: chalk.cyan('Confirm encryption password:'), | ||
| mask: '*', | ||
| validate: (input: string) => { | ||
| // eslint-disable-next-line security/detect-possible-timing-attacks | ||
| if (input !== password) { | ||
| return chalk.red('Passwords do not match'); | ||
| } | ||
| return true; | ||
| }, | ||
| }, | ||
| ]); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unused variable _confirmPassword is assigned but never used. While it's prefixed with an underscore to indicate intentional non-use, this assignment is unnecessary since the validation already occurs in the prompt's validate function. Consider removing the destructuring assignment entirely.
| console.log(chalk.yellow('\nSetup cancelled.')); | ||
| process.exit(0); | ||
| } | ||
| console.error(chalk.red('Failed to start agent:'), error); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message "Failed to start agent:" doesn't provide sufficient context about what went wrong. Consider logging more details about the error or adding guidance on how to resolve common issues. This is especially important since this is the main entry point error handler.
| console.error(chalk.red('Failed to start agent:'), error); | |
| console.error(chalk.red('Failed to start agent.')); | |
| if (error && typeof error === 'object') { | |
| if ('message' in error) { | |
| console.error(chalk.red('Error message:'), (error as Error).message); | |
| } | |
| if ('stack' in error) { | |
| console.error(chalk.gray('Stack trace:'), (error as Error).stack); | |
| } | |
| } else { | |
| console.error(chalk.red('Error:'), error); | |
| } | |
| console.error( | |
| chalk.yellow( | |
| '\nCommon issues:\n' + | |
| ' • Check your configuration files and environment variables.\n' + | |
| ' • Ensure network connectivity to the WebSocket server.\n' + | |
| ' • Verify database credentials and permissions.\n' + | |
| ' • For more help, visit https://docs.rocketadmin.com or contact support.\n' | |
| ) | |
| ); |
No description provided.