Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
language: "en-US"

tone_instructions: |
You are a Senior Software Engineer and Security Specialist.
When reviewing, focus on:
1. **Security**: Ensure no secrets are leaked and input validation is robust (especially for URLs and ports).
2. **Performance**: Look for inefficient async operations (e.g., Promise.all vs. Promise.allSettled).
3. **TUI/UX**: Since this is a CLI tool, ensure the interactive flows are intuitive and handle edge cases like cancellations.
4. **Test Coverage**: Ensure all new logic is covered by vitest unit tests.

reviews:
profile: "assertive"
high_level_summary: true
auto_review:
enabled: true
drafts: false

path_filters:
- "!dist/**"
- "!node_modules/**"
- "!package-lock.json"
- "src/**"
- ".github/workflows/**"

path_instructions:
- path: "src/commands/**/*.ts"
instructions: |
- Verify that all commands follow the modular registration pattern.
- Ensure errors are handled gracefully and logged using the project's logger.
- For TUI interactions, verify that `@vr_patel/tui` tools are used correctly.
- path: "src/utils/config.ts"
instructions: |
- Ensure configuration keys are type-safe.
- Verify that sensitive information is not stored in plain text if possible.
- path: "src/__tests__/**/*.ts"
instructions: |
- Ensure mocks are clean and shared correctly.
- Verify that tests cover both happy paths and error conditions.

pre_merge_checks:
linked_issue_assessment:
mode: "warning"
12 changes: 11 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"license": "MIT",
"dependencies": {
"@kubernetes/client-node": "^0.20.0",
"@vr_patel/tui": "^1.0.0",
"chalk": "^5.3.0",
"cli-table3": "^0.6.3",
"commander": "^12.0.0",
Expand All @@ -53,7 +54,7 @@
"@types/dockerode": "^3.3.26",
"@types/node": "^20.11.24",
"@types/nodemailer": "^8.0.0",
"@types/react": "^18.2.63",
"@types/react": "^18.3.28",
"tsup": "^8.0.2",
"typescript": "^5.3.3",
"vitest": "^1.3.1"
Expand Down
69 changes: 61 additions & 8 deletions src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { Command } from 'commander';
import { registerConfigCommand } from '../commands/config';
import { setConfig, getConfig, clearConfig } from '../utils/config';
import * as configUtils from '../utils/config';
import * as tui from '@vr_patel/tui';

// Mock the modules
vi.mock('../utils/config', () => ({
setConfig: vi.fn(),
getConfig: vi.fn(() => ({})),
clearConfig: vi.fn(),
clearNotificationCredentials: vi.fn(),
}));

vi.mock('@vr_patel/tui', () => ({
select: vi.fn(),
input: vi.fn(),
}));

describe('config command', () => {
Expand All @@ -22,34 +30,79 @@ describe('config command', () => {
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
});

it('should register config set, list, and clear commands', () => {
it('should register config setup, set, list, and clear commands', () => {
const configCmd = program.commands.find((c) => c.name() === 'config');
expect(configCmd).toBeDefined();
expect(configCmd?.commands.map((c) => c.name())).toEqual(['set', 'list', 'clear']);
const subCommandNames = configCmd?.commands.map((c) => c.name());
expect(subCommandNames).toContain('setup');
expect(subCommandNames).toContain('set');
expect(subCommandNames).toContain('list');
expect(subCommandNames).toContain('clear');
});

it('should clear credentials and set service to none', async () => {
vi.mocked(tui.select).mockResolvedValue('none');

await program.parseAsync(['node', 'test', 'config', 'setup']);

expect(tui.select).toHaveBeenCalled();
expect(configUtils.clearNotificationCredentials).toHaveBeenCalled();
expect(configUtils.setConfig).toHaveBeenCalledWith('notification_service', 'none');
expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringMatching(/Notifications disabled/i));
});

it('should call select, input and setConfig on discord setup', async () => {
vi.mocked(tui.select).mockResolvedValue('discord');
vi.mocked(tui.input).mockResolvedValue('https://discord.com/api/webhooks/123456789/token-here');

await program.parseAsync(['node', 'test', 'config', 'setup']);
Comment on lines +54 to +58

expect(tui.select).toHaveBeenCalled();
expect(tui.input).toHaveBeenCalled();
expect(configUtils.clearNotificationCredentials).toHaveBeenCalled();
expect(configUtils.setConfig).toHaveBeenCalledWith('notification_service', 'discord');
expect(configUtils.setConfig).toHaveBeenCalledWith('discord_webhook', 'https://discord.com/api/webhooks/123456789/token-here');
});

it('should call select, multiple inputs and setConfig on email setup', async () => {
vi.mocked(tui.select).mockResolvedValue('email');
vi.mocked(tui.input)
.mockResolvedValueOnce('smtp.gmail.com') // host
.mockResolvedValueOnce('587') // port
.mockResolvedValueOnce('user@test.com') // user
.mockResolvedValueOnce('to@test.com'); // to

await program.parseAsync(['node', 'test', 'config', 'setup']);

expect(configUtils.setConfig).toHaveBeenCalledWith('notification_service', 'email');
expect(configUtils.setConfig).toHaveBeenCalledWith('email_host', 'smtp.gmail.com');
expect(configUtils.setConfig).toHaveBeenCalledWith('email_port', 587);
expect(configUtils.setConfig).toHaveBeenCalledWith('email_user', 'user@test.com');
expect(configUtils.setConfig).toHaveBeenCalledWith('email_to', 'to@test.com');
});
Comment on lines +67 to 82
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert credential cleanup for the email setup path.

Line 67-82 validates email field persistence, but it doesn’t verify that configUtils.clearNotificationCredentials() is called when switching services. This leaves the cleanup requirement only partially covered.

Suggested test adjustment
   it('should call select, multiple inputs and setConfig on email setup', async () => {
     vi.mocked(tui.select).mockResolvedValue('email');
     vi.mocked(tui.input)
       .mockResolvedValueOnce('smtp.gmail.com') // host
       .mockResolvedValueOnce('587')            // port
       .mockResolvedValueOnce('user@test.com')  // user
       .mockResolvedValueOnce('to@test.com');    // to

     await program.parseAsync(['node', 'test', 'config', 'setup']);

+    expect(tui.select).toHaveBeenCalled();
+    expect(tui.input).toHaveBeenCalledTimes(4);
+    expect(configUtils.clearNotificationCredentials).toHaveBeenCalled();
     expect(configUtils.setConfig).toHaveBeenCalledWith('notification_service', 'email');
     expect(configUtils.setConfig).toHaveBeenCalledWith('email_host', 'smtp.gmail.com');
     expect(configUtils.setConfig).toHaveBeenCalledWith('email_port', 587);
     expect(configUtils.setConfig).toHaveBeenCalledWith('email_user', 'user@test.com');
     expect(configUtils.setConfig).toHaveBeenCalledWith('email_to', 'to@test.com');
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/__tests__/config.test.ts` around lines 67 - 82, The test currently
verifies setting email fields but doesn't assert that previous notification
credentials are cleared; update the 'should call select, multiple inputs and
setConfig on email setup' test to also assert that
configUtils.clearNotificationCredentials() is invoked when switching services:
keep the existing mocks for tui.select and tui.input, run
program.parseAsync(['node','test','config','setup']) as before, then add an
expectation like
expect(configUtils.clearNotificationCredentials).toHaveBeenCalled() after the
parse to ensure credentials cleanup occurs alongside the setConfig assertions
for 'notification_service', 'email_host', 'email_port', 'email_user', and
'email_to'.


it('should call setConfig on config set', async () => {
await program.parseAsync(['node', 'test', 'config', 'set', 'alert_email', 'test@test.com']);
expect(setConfig).toHaveBeenCalledWith('alert_email', 'test@test.com');
expect(configUtils.setConfig).toHaveBeenCalledWith('alert_email', 'test@test.com');
expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Set alert_email to test@test.com'));
});

it('should parse integer for alert_cooldown', async () => {
await program.parseAsync(['node', 'test', 'config', 'set', 'alert_cooldown', '123']);
expect(setConfig).toHaveBeenCalledWith('alert_cooldown', 123);
expect(configUtils.setConfig).toHaveBeenCalledWith('alert_cooldown', 123);
});

it('should call getConfig on config list', async () => {
(getConfig as any).mockReturnValue({ alert_cooldown: 100 });
vi.mocked(configUtils.getConfig).mockReturnValue({ alert_cooldown: 100 });
await program.parseAsync(['node', 'test', 'config', 'list']);
expect(getConfig).toHaveBeenCalled();
expect(configUtils.getConfig).toHaveBeenCalled();
expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('alert_cooldown'));
expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('100'));
});

it('should call clearConfig on config clear', async () => {
await program.parseAsync(['node', 'test', 'config', 'clear']);
expect(clearConfig).toHaveBeenCalled();
expect(configUtils.clearConfig).toHaveBeenCalled();
expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Configuration cleared'));
});
});
4 changes: 2 additions & 2 deletions src/__tests__/show.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { getRunningContainers } from '../docker/containers';
import { getRunningPods } from '../kubernetes/pods';
import { logger } from '../utils/logger';
import { showRunners, showPods } from '../commands/show';
import * as tableUtils from '../ui/table';

// Mock dependencies
vi.mock('../docker/containers', () => ({
Expand Down Expand Up @@ -43,8 +44,7 @@ describe('show command runners', () => {
// Verify: Warning was logged for Kubernetes, but no crash
expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Kubernetes is unreachable'));
// Verify: Table still rendered with Docker data
const { renderTable } = await import('../ui/table');
expect(renderTable).toHaveBeenCalled();
expect(tableUtils.renderTable).toHaveBeenCalled();
});

it('should handle both services failing gracefully in showRunners', async () => {
Expand Down
77 changes: 76 additions & 1 deletion src/commands/config.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,87 @@
import { Command } from 'commander';
import chalk from 'chalk';
import { setConfig, getConfig, clearConfig } from '../utils/config';
import { setConfig, getConfig, clearConfig, clearNotificationCredentials } from '../utils/config';
import { select, input } from '@vr_patel/tui';

export const registerConfigCommand = (program: Command) => {
const config = program
.command('config')
.description('Manage KDM configuration');

config
.command('setup')
.description('Interactively set up notification service')
.action(async () => {
try {
const choice = await select({
message: 'Select notification service:',
options: [
{ label: 'Discord', value: 'discord', description: 'Send alerts to a Discord channel via Webhook' },
{ label: 'Email (SMTP)', value: 'email', description: 'Send alerts via Email SMTP' },
{ label: 'None', value: 'none', description: 'Disable notifications' },
],
});

if (choice === 'none') {
clearNotificationCredentials();
setConfig('notification_service', 'none');
Comment on lines +25 to +27
console.log(chalk.green('\n✓ Notifications disabled.'));
return;
}

if (choice === 'discord') {
const webhook = await input({
message: 'Discord Webhook URL:',
validate: (v) => {
const discordWebhookRegex = /^https:\/\/(?:ptb\.|canary\.)?discord\.com\/api\/webhooks\/\d+\/[\w-]+$/;
return discordWebhookRegex.test(v) || 'Must be a valid Discord webhook URL (including ID and Token)';
},
});
Comment on lines +33 to +39

clearNotificationCredentials();
setConfig('discord_webhook', webhook);
setConfig('notification_service', 'discord');
console.log(chalk.green('\n✓ Discord Webhook configured.'));
} else if (choice === 'email') {
const host = await input({
message: 'SMTP Host:',
placeholder: 'smtp.gmail.com',
validate: (v) => v.length > 0 || 'Host is required',
});
const portStr = await input({
message: 'SMTP Port:',
defaultValue: '587',
validate: (v) => {
const port = parseInt(v, 10);
return (/^\d+$/.test(v) && port > 0 && port <= 65535) || 'Must be a valid port number (1-65535)';
},
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const user = await input({
message: 'SMTP User:',
validate: (v) => /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(v) || 'Must be a valid email address',
});
const to = await input({
message: 'Alert Recipient Email:',
validate: (v) => /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(v) || 'Must be a valid email address',
});

clearNotificationCredentials();
setConfig('email_host', host);
setConfig('email_port', parseInt(portStr, 10));
setConfig('email_user', user);
setConfig('email_to', to);
setConfig('notification_service', 'email');

console.log(chalk.green('\n✓ Email SMTP configured.'));
console.log(chalk.yellow('! Important: Set your SMTP password in the KDM_SMTP_PASSWORD environment variable for notifications to work.'));
}

console.log(chalk.green(`✓ Notification service set to: ${chalk.bold(choice.toUpperCase())}`));
} catch (error) {
console.error(chalk.red(`✗ Set up cancelled or failed: ${(error as Error).message}`));
}
});

config
.command('set <key> <value>')
.description('Set a configuration value')
Expand Down
9 changes: 9 additions & 0 deletions src/utils/config.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Conf from 'conf';

interface KDMConfig {
notification_service?: 'discord' | 'email' | 'none';
discord_webhook?: string;
email_host?: string;
email_port?: number;
Expand All @@ -18,6 +19,14 @@ export const setConfig = (key: keyof KDMConfig, value: any) => config.set(key, v
export const deleteConfig = (key: keyof KDMConfig) => config.delete(key);
export const clearConfig = () => config.clear();

export const clearNotificationCredentials = () => {
config.delete('discord_webhook');
config.delete('email_host');
config.delete('email_port');
config.delete('email_user');
config.delete('email_to');
};

// Helper for sensitive data - always use environment variables
export const getSMTPSettings = () => {
return {
Expand Down
Loading