Phase 1: Make KDM an AI CLI tool for K8s #92
Open
utkarsh232005 wants to merge 1 commit into
Open
Conversation
Contributor
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Configuration schema and defaults src/config/schema.ts |
Defines KDMConfig, sub-configs (AI, Kubernetes, Cache, Output, Notification), LegacyNotificationConfig, and defaultKDMConfig with defaults for activeFilters, cache, and output. |
Legacy notification migration functions src/config/migration.ts, src/__tests__/config-migration.test.ts |
Adds notificationFromLegacy to convert legacy snake_case fields to NotificationConfig (or return undefined), and mergeLegacyConfig to compose final KDMConfig applying legacy-derived notifications before stored notifications; tests validate mapping, malformed-value handling, and precedence. |
Centralized configuration store src/config/store.ts, src/__tests__/config-store.test.ts |
Implements Conf<StoredKDMConfig> with withDefaults (defaults + legacy migrations + persisted values), typed getters/setters for ai, activeFilters, kubernetes, cache, output, and notifications; exposes getLegacyConfig/getLegacyValue/setLegacyValue/deleteLegacyValue and rawConfigStore; tests cover defaults, persistence, migration roundtrip, and safe defaults for malformed legacy values. |
Integration with existing utils src/utils/config.ts, src/__tests__/config-utils.test.ts |
Replaces direct conf usage with the new store helpers. setConfig now rejects storing the sensitive legacy email_password (throws, directs to KDM_SMTP_PASSWORD). getSMTPSettings reads values via getLegacyValue and prefers process.env.KDM_SMTP_PASSWORD for the password. Tests assert rejection and no persistence of email_password. |
CLI SMTP flow & messaging src/commands/config.ts, src/__tests__/config.test.ts |
Removes SMTP password prompt and persistence from interactive setup; clears prior notification credentials; updates config list note and SMTP guide to instruct using KDM_SMTP_PASSWORD; tests updated to reflect omitted prompt, validate prompt behavior, and confirm email_password is not saved. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- KDM-cli/kdm-cli#91: Overlaps SMTP/email config handling and
KDM_SMTP_PASSWORDbehavior (similar changes tosrc/utils/config.tsand CLI flow). - KDM-cli/kdm-cli#23: Related interactive
config setupmodifications affecting SMTP prompts and guidance. - KDM-cli/kdm-cli#14: Prior changes to interactive notification setup and credential persistence that this PR refactors to use migration-aware store.
Suggested reviewers
- Rishiraj-Pathak-27
Poem
⚙️ The Config Migration Rhyme
Old snake_case whispers fade away,
CamelCase hums in typed array,
Defaults align and secrets hide,
Env var keeps the password tied,
Tests sing safe, migrations play.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Title check | ✅ Passed | The PR title 'Phase 1: Make KDM an AI CLI tool for K8s' directly aligns with the PR objectives describing Phase 1 configuration foundation for evolving KDM into an AI-assisted Kubernetes CLI. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/__tests__/config-migration.test.ts`:
- Around line 4-44: Add a new test asserting that notificationFromLegacy({})
returns undefined to cover the no-legacy-input/error path: create an it block
(e.g., "returns undefined for empty legacy input") that calls
notificationFromLegacy({}) and expects the result toBeUndefined(); keep this
separate from the existing happy-path tests and similarly verify
mergeLegacyConfig still prefers explicit notifications when provided (no change
to mergeLegacyConfig behavior).
In `@src/__tests__/config-store.test.ts`:
- Around line 46-96: Add a new test case that exercises an error/edge path for
legacy notification handling: call setLegacyValue with a corrupted/invalid shape
(e.g., setLegacyValue('notification_service', { invalid: 'object' }) or
setLegacyValue('alert_cooldown', 'not-a-number')) or simulate removing a
required legacy key, then call getLegacyConfig() (or the relevant getter) and
assert the module either returns safe defaults/undefineds for the malformed
fields or throws a specific error; update the test suite to use the same helpers
(setLegacyValue, getLegacyConfig, clearConfig) and include assertions that
validate the code's defensive behavior for malformed legacy data so coverage
includes an explicit failure/edge path.
In `@src/config/store.ts`:
- Around line 85-87: The getLegacyValue function currently calls config.get(key)
and thus misses values that were migrated into the newer notifications.*
projection; change getLegacyValue to read from getLegacyConfig() (e.g., const
legacy = getLegacyConfig()) and return legacy[key] so legacy/new notification
values are preserved for legacy readers; keep the same generic signature (Key
extends keyof LegacyNotificationConfig) and ensure you reference
getLegacyConfig, getLegacyValue, LegacyNotificationConfig and avoid using
config.get inside getLegacyValue.
In `@src/utils/config.ts`:
- Line 11: The setConfig wrapper currently allows persisting sensitive keys
(e.g. email_password) via setLegacyValue; modify setConfig (and any callers that
write notification config) to block or reject writes for sensitive keys listed
in LegacyNotificationConfig such as "email_password" and require these secrets
be injected from env or an encrypted secret store instead—update setConfig to
detect those keys, throw or return an error when attempted, and add
guidance/logging pointing callers to use the environment override (or an
encrypted secret manager) rather than persisting plaintext credentials.
- Line 11: Change setConfig to be type-safe by making it generic over the config
key so the value type is inferred from LegacyNotificationConfig: replace the
loose signature setConfig(key: keyof LegacyNotificationConfig, value: any) with
a generic form like <K extends keyof LegacyNotificationConfig>(key: K, value:
LegacyNotificationConfig[K]) and forward those types to setLegacyValue (adjust
its invocation/type parameters if needed) so callers cannot pass values that
don't match the keyed config type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 308ab63e-11b5-47cc-84b0-94fba27f94fb
📒 Files selected for processing (6)
src/__tests__/config-migration.test.tssrc/__tests__/config-store.test.tssrc/config/migration.tssrc/config/schema.tssrc/config/store.tssrc/utils/config.ts
# Conflicts: # src/utils/config.ts
utkarsh232005
force-pushed
the
feat/kdm-ai-config-phase1
branch
from
24781f0 to
e02fe03
Compare
utkarsh232005
changed the title
utkarsh232005
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/config.test.ts (1)
92-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse key-based assertions so any
email_passwordpersistence fails the test.
not.toHaveBeenCalledWith('email_password', expect.any(String))misses non-string writes. Assert that no call uses theemail_passwordkey at all.Suggested fix
- expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password', expect.any(String)); + expect( + vi.mocked(configUtils.setConfig).mock.calls.some(([key]) => key === 'email_password'), + ).toBe(false); @@ - expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password', expect.any(String)); + expect( + vi.mocked(configUtils.setConfig).mock.calls.some(([key]) => key === 'email_password'), + ).toBe(false);🤖 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 92 - 128, Replace the current value-type assertion for email_password with a key-based check so any persistence (of any type) fails the test: update both occurrences of expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password', expect.any(String)) to expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password', expect.anything()). Keep the rest of the test (tui.select/tui.input mocks, program.parseAsync, consoleLogSpy assertions) unchanged.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/__tests__/config-utils.test.ts`:
- Around line 95-98: The test currently only forbids writing the specific value
'secret' for the 'email_password' key; update the negative assertion so it
rejects any write to that key regardless of value. In the test that calls
setConfig('email_password', 'secret'), replace the assertion against
mockConfInstance.set that references the exact value with one that asserts
mockConfInstance.set was not called with the key 'email_password' and any value
(e.g., using expect.anything()) so the check targets the key rather than a
single value; keep the existing expect(...).toThrow(/KDM_SMTP_PASSWORD/) and the
call to setConfig in place.
In `@src/config/migration.ts`:
- Line 7: The migration currently uses numberOrUndefined which accepts any
number (including negatives) and lets invalid values like negative alertCooldown
or out-of-range smtpPort through; update the migration to validate numeric
fields before accepting them: replace or extend numberOrUndefined to perform
range checks (e.g., non-negative for alertCooldown/timeout fields, valid TCP
port range 1-65535 for smtpPort) or add field-specific validators where numbers
are parsed (referencing numberOrUndefined and the migration code paths that
assign alertCooldown and smtpPort) so only numbers within the allowed ranges are
returned; on invalid values return undefined (or fallback defaults) and log or
record the rejection.
In `@src/config/store.ts`:
- Around line 35-39: The updateConfig mutator currently writes all keys from
nextConfig into the persistent store (in updateConfig), which allows SMTP
password fields to be persisted and violates the env-var-only policy; modify
updateConfig to detect SMTP password keys (e.g., key names like "smtpPassword",
"smtp_password", or any key matching a case-insensitive /smtp.*pass/ pattern)
and skip calling config.set for those keys (or explicitly set them to
undefined/null) so they are never written to the store; apply the same guard to
any other store-level mutators that call config.set (refer to the same key-name
checks) to ensure all code paths block SMTP password persistence.
In `@src/utils/config.ts`:
- Line 10: getConfig currently returns the raw legacy config (via
getLegacyConfig()) which exposes the legacy email_password; change getConfig to
call getLegacyConfig(), then remove or mask the email_password field before
returning (e.g., omit the key or replace value with a fixed mask), while leaving
getSMTPSettings() unchanged so it remains the only accessor that can return the
real SMTP password; ensure you return a shallow/deep copy of the config object
(not mutate the original) and reference getConfig, getLegacyConfig,
getSMTPSettings, and the email_password property when making the change.
---
Outside diff comments:
In `@src/__tests__/config.test.ts`:
- Around line 92-128: Replace the current value-type assertion for
email_password with a key-based check so any persistence (of any type) fails the
test: update both occurrences of
expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password',
expect.any(String)) to
expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password',
expect.anything()). Keep the rest of the test (tui.select/tui.input mocks,
program.parseAsync, consoleLogSpy assertions) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc0b2d15-7d4d-4bdd-9ad3-84277d697ba4
📒 Files selected for processing (9)
src/__tests__/config-migration.test.tssrc/__tests__/config-store.test.tssrc/__tests__/config-utils.test.tssrc/__tests__/config.test.tssrc/commands/config.tssrc/config/migration.tssrc/config/schema.tssrc/config/store.tssrc/utils/config.ts
Comment on lines
+95
to
+98
| it('should reject storing SMTP passwords in config', () => { | ||
| expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/); | ||
| expect(mockConfInstance.set).not.toHaveBeenCalledWith('email_password', 'secret'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Strengthen the negative assertion to reject any email_password write.
The current check only blocks one exact value ('secret'). A write with the same key and a different value would still pass.
Suggested fix
it('should reject storing SMTP passwords in config', () => {
expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/);
- expect(mockConfInstance.set).not.toHaveBeenCalledWith('email_password', 'secret');
+ expect(
+ mockConfInstance.set.mock.calls.some(([key]: [string]) => key === 'email_password'),
+ ).toBe(false);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should reject storing SMTP passwords in config', () => { | |
| expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/); | |
| expect(mockConfInstance.set).not.toHaveBeenCalledWith('email_password', 'secret'); | |
| }); | |
| it('should reject storing SMTP passwords in config', () => { | |
| expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/); | |
| expect( | |
| mockConfInstance.set.mock.calls.some(([key]: [string]) => key === 'email_password'), | |
| ).toBe(false); | |
| }); |
🤖 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-utils.test.ts` around lines 95 - 98, The test currently
only forbids writing the specific value 'secret' for the 'email_password' key;
update the negative assertion so it rejects any write to that key regardless of
value. In the test that calls setConfig('email_password', 'secret'), replace the
assertion against mockConfInstance.set that references the exact value with one
that asserts mockConfInstance.set was not called with the key 'email_password'
and any value (e.g., using expect.anything()) so the check targets the key
rather than a single value; keep the existing
expect(...).toThrow(/KDM_SMTP_PASSWORD/) and the call to setConfig in place.
| value === 'discord' || value === 'email' || value === 'none'; | ||
|
|
||
| const stringOrUndefined = (value: unknown) => (typeof value === 'string' ? value : undefined); | ||
| const numberOrUndefined = (value: unknown) => (typeof value === 'number' ? value : undefined); |
Contributor
There was a problem hiding this comment.
Validate migrated numeric fields before accepting them.
On Line 30 and Line 34, any numeric value is accepted, including negatives/out-of-range values. That can disable cooldown behavior downstream (e.g., negative alertCooldown) and accept invalid SMTP ports.
Suggested fix
-const numberOrUndefined = (value: unknown) => (typeof value === 'number' ? value : undefined);
+const finiteNumberOrUndefined = (value: unknown) =>
+ typeof value === 'number' && Number.isFinite(value) ? value : undefined;
+
+const emailPortOrUndefined = (value: unknown) => {
+ const n = finiteNumberOrUndefined(value);
+ return n !== undefined && Number.isInteger(n) && n >= 1 && n <= 65535 ? n : undefined;
+};
+
+const alertCooldownOrUndefined = (value: unknown) => {
+ const n = finiteNumberOrUndefined(value);
+ return n !== undefined && n >= 0 ? n : undefined;
+};
@@
- emailPort: numberOrUndefined(legacy.email_port),
+ emailPort: emailPortOrUndefined(legacy.email_port),
@@
- alertCooldown: numberOrUndefined(legacy.alert_cooldown),
+ alertCooldown: alertCooldownOrUndefined(legacy.alert_cooldown),Also applies to: 30-35
🤖 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/config/migration.ts` at line 7, The migration currently uses
numberOrUndefined which accepts any number (including negatives) and lets
invalid values like negative alertCooldown or out-of-range smtpPort through;
update the migration to validate numeric fields before accepting them: replace
or extend numberOrUndefined to perform range checks (e.g., non-negative for
alertCooldown/timeout fields, valid TCP port range 1-65535 for smtpPort) or add
field-specific validators where numbers are parsed (referencing
numberOrUndefined and the migration code paths that assign alertCooldown and
smtpPort) so only numbers within the allowed ranges are returned; on invalid
values return undefined (or fallback defaults) and log or record the rejection.
Comment on lines
+35
to
+39
| export const updateConfig = (nextConfig: KDMConfig) => { | ||
| Object.entries(nextConfig).forEach(([key, value]) => { | ||
| config.set(key as keyof StoredKDMConfig, value as never); | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Block SMTP password persistence in store-level mutators.
Line 37, Line 42, and Line 96 can still persist SMTP password fields, which bypasses the env-var-only policy. Enforce the same guard at this layer so all call paths are safe.
Suggested fix
+const assertNoStoredSmtpPassword = (key: string, value: unknown) => {
+ if (key === 'email_password') {
+ throw new Error('Use KDM_SMTP_PASSWORD instead of storing email_password in config');
+ }
+ if (
+ key === 'notifications' &&
+ value &&
+ typeof value === 'object' &&
+ 'emailPassword' in value &&
+ (value as { emailPassword?: string }).emailPassword !== undefined
+ ) {
+ throw new Error('Use KDM_SMTP_PASSWORD instead of storing notifications.emailPassword in config');
+ }
+};
+
export const updateConfig = (nextConfig: KDMConfig) => {
Object.entries(nextConfig).forEach(([key, value]) => {
+ assertNoStoredSmtpPassword(key, value);
config.set(key as keyof StoredKDMConfig, value as never);
});
};
@@
export const setConfigValue = <Key extends keyof KDMConfig>(key: Key, value: KDMConfig[Key]) => {
+ assertNoStoredSmtpPassword(String(key), value);
config.set(key as keyof StoredKDMConfig, value as never);
};
@@
export const setLegacyValue = <Key extends keyof LegacyNotificationConfig>(
key: Key,
value: LegacyNotificationConfig[Key],
) => {
+ assertNoStoredSmtpPassword(String(key), value);
config.set(key, value as never);
};Also applies to: 41-43, 92-97
🤖 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/config/store.ts` around lines 35 - 39, The updateConfig mutator currently
writes all keys from nextConfig into the persistent store (in updateConfig),
which allows SMTP password fields to be persisted and violates the env-var-only
policy; modify updateConfig to detect SMTP password keys (e.g., key names like
"smtpPassword", "smtp_password", or any key matching a case-insensitive
/smtp.*pass/ pattern) and skip calling config.set for those keys (or explicitly
set them to undefined/null) so they are never written to the store; apply the
same guard to any other store-level mutators that call config.set (refer to the
same key-name checks) to ensure all code paths block SMTP password persistence.
| email_password?: string; | ||
| alert_cooldown?: number; // in seconds | ||
| } | ||
| export const getConfig = () => getLegacyConfig(); |
Contributor
There was a problem hiding this comment.
Hide legacy SMTP passwords from the public config view.
getConfig() now forwards getLegacyConfig() unchanged, and that object still includes email_password. Downstream, the config list command prints every entry verbatim, so any pre-existing legacy SMTP password will still be exposed in the terminal even though new writes are blocked. Keep password access limited to getSMTPSettings() and mask or omit email_password from getConfig().
As per coding guidelines, "Verify that sensitive information is not stored in plain text if possible."
🤖 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/utils/config.ts` at line 10, getConfig currently returns the raw legacy
config (via getLegacyConfig()) which exposes the legacy email_password; change
getConfig to call getLegacyConfig(), then remove or mask the email_password
field before returning (e.g., omit the key or replace value with a fixed mask),
while leaving getSMTPSettings() unchanged so it remains the only accessor that
can return the real SMTP password; ensure you return a shallow/deep copy of the
config object (not mutate the original) and reference getConfig,
getLegacyConfig, getSMTPSettings, and the email_password property when making
the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
This prepares KDM for the next phases: analyzer engine, K8s issue detection, AI provider auth, filters, cache, and AI explanations.
Testing
Summary by CodeRabbit
Tests
Refactor
Bug Fix / UX