feat(analysis): implement phase 2 analyzer engine and registry#96
Open
utkarsh232005 wants to merge 2 commits into
Open
feat(analysis): implement phase 2 analyzer engine and registry#96utkarsh232005 wants to merge 2 commits into
utkarsh232005 wants to merge 2 commits into
Conversation
# Conflicts: # src/utils/config.ts
Contributor
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Configuration schema and defaults src/config/schema.ts |
Defines TypeScript interfaces for AI, Kubernetes, cache, output, and notification configuration sections, plus a legacy notification shape and sensible defaults for activeFilters, file caching, and text output language. |
Persistent configuration store with typed accessors src/config/store.ts |
Implements a centralized conf-backed store that materializes config with defaults, provides typed getters/setters for each config section, maps modern notification config to legacy schema for backward compatibility, and exposes legacy-specific read/write/delete helpers. |
Legacy configuration migration src/config/migration.ts |
Provides utilities to map legacy snake_case notification fields to modern camelCase, normalize missing service to 'none', and merge migrated legacy settings with stored modern config. |
Analyzer framework: types and registry src/analyzers/types.ts, src/analyzers/index.ts |
Defines Analyzer, AnalyzerContext, AnalyzerResult, Failure, and SensitiveValue interfaces; introduces AnalyzerRegistry and a singleton registry instance; exports five placeholder no-op analyzers ready for implementation. |
Analysis types and orchestration src/analysis/types.ts, src/analysis/stats.ts, src/analysis/analysis.ts |
Defines AnalysisOptions, AnalysisStats, and AnalysisOutput types; implements measureDuration timing utility; orchestrates runAnalysis to resolve filters, validate concurrency limits, execute analyzers in a worker pool, collect per-analyzer stats and errors, compute aggregate problem counts, and safely attach AI provider config. |
Analysis output formatting src/analysis/output.ts |
Provides formatTextOutput (colorized grouped report with status, provider, errors, results, and optional stats) and formatJsonOutput (pretty-printed JSON serialization). |
Secure email credential handling src/utils/config.ts, src/commands/config.ts |
Refactors credential utilities to reject email_password storage, requiring the KDM_SMTP_PASSWORD environment variable instead; removes password prompt from email setup flow and updates guidance accordingly. |
Comprehensive test suite src/__tests__/config-migration.test.ts, src/__tests__/config-store.test.ts, src/__tests__/config-utils.test.ts, src/__tests__/config.test.ts, src/__tests__/analysis.test.ts |
Mocks persistent storage, validates config persistence and legacy migration round-trips, verifies credential security, and tests analysis orchestration including concurrency limits, per-analyzer error resilience, stats collection, and output formatting. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- KDM-cli/kdm-cli#23: Updates
src/commands/config.tsemail setup guidance andKDM_SMTP_PASSWORDmessaging. - KDM-cli/kdm-cli#91: Modifies SMTP credential handling in
src/utils/config.tsaroundemail_passwordand environment-variable precedence.
Suggested reviewers
- Rishiraj-Pathak-27
Poem
🔐 From chaos of config to order and schema,
A registry of analyzers, orchestrated just so,
Passwords flee to the env, no storage in sight,
Concurrency pooled and stats counted right,
Legacy gracefully migrated to light. ✨
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title clearly and concisely summarizes the main objective: implementing Phase 2 of the analyzer engine and registry system. |
| 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. |
✏️ 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: 10
🤖 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__/analysis.test.ts`:
- Around line 33-37: Tests are leaking a shared analyzer registry state because
only clearConfig() is called in beforeEach; update the test setup to also reset
the analyzer registry between tests by calling the registry reset method (e.g.,
registry.clear() or registry.reset()) in beforeEach (and/or afterEach) so any
registry.register(...) calls are undone; if no reset method exists, add a
registry.reset/clear implementation on the registry singleton and call that from
the test beforeEach blocks where registry.register is used (references:
registry.register).
- Around line 84-119: The test using real setTimeouts in the 'respects
concurrency limit settings' case is flaky; replace the time-based delays with
controlled promises so concurrency is deterministic: create controllable promise
resolvers for delayAnalyzer1 and delayAnalyzer2 (referenced as delayAnalyzer1,
delayAnalyzer2) that increment/decrement activeCalls inside their analyze
methods and await their respective externally-resolved promises, register them
via registry.register(...), call runAnalysis({ filters: ['Delay1','Delay2'],
maxConcurrency: 1 }), then resolve the promises in the desired order to assert
maxConcurrentCalls === 1; ensure any test-level mocks or fake timers are
cleaned/reset per the project test guidelines.
In `@src/__tests__/config-migration.test.ts`:
- Around line 33-47: Add a new unit test in
src/__tests__/config-migration.test.ts that covers the error path of
mergeLegacyConfig: call mergeLegacyConfig with malformed/invalid legacy fields
(e.g., notification_service set to an unexpected value or discord_webhook set to
a non-string) and assert the function safely falls back to the default
notifications object (for example notifications.service === 'none' and
notifications.discordWebhook undefined). Reference mergeLegacyConfig in the test
name and assertions so reviewers can find the handling for legacy -> new mapping
and validate the safe fallback behavior.
In `@src/__tests__/config-store.test.ts`:
- Line 21: Remove the global leak by deleting the assignment "(globalThis as
any).mockConfigStore = mockConfigStore" and keep mockConfigStore as a
closure-scoped test variable; if other suites need access, expose it via a
dedicated test helper or setup file (e.g., export a getMockConfigStore helper or
register it in jest.setup.ts) rather than writing to globalThis so mock state
stays isolated to the test scope.
In `@src/analysis/analysis.ts`:
- Around line 85-86: The status variable is incorrectly based on results.length
instead of the computed problems count, causing mismatches; change the status
assignment to use the already computed problems (from results.reduce) so it
becomes status = problems > 0 ? 'ProblemDetected' : 'OK' ensuring the
declaration order keeps problems computed before status (refer to the problems
variable and the status assignment near results.reduce).
In `@src/analysis/output.ts`:
- Around line 48-52: The loop that renders failures prints raw values from
failure.text and failure.kubernetesDoc which can leak secrets when
failure.sensitive is set; update the rendering in the for (const failure of
res.errors) block to pass those strings through a redaction helper before
pushing to lines (e.g., create or use a function named redactSensitive(value:
string, sensitive?: any) and call lines.push(` - ${chalk.red('Error:')}
${chalk.red(redactSensitive(failure.text, failure.sensitive))}`) and similarly
for failure.kubernetesDoc), or always run redactSensitive on those fields to
ensure sensitive data is redacted before output.
In `@src/config/migration.ts`:
- Around line 46-49: The merged notifications object created with
"notifications: { ...legacyNotifications, ...stored.notifications }" can become
an empty object and lack the required "service" field; change the code to
compute the mergedNotifications (using legacyNotifications and
stored.notifications) and only include the notifications key when
mergedNotifications has meaningful content (e.g., mergedNotifications.service is
present or Object.keys(mergedNotifications).length > 0), otherwise set
notifications to undefined (or explicitly provide a default service) so callers
never receive an empty {}.
In `@src/config/store.ts`:
- Around line 68-69: The notifications SMTP password must not be persisted or
returned: update setNotificationConfig to remove/omit
notifications.emailPassword (and any email_password field) before calling
setConfigValue('notifications', ...), and update the corresponding getter (e.g.,
getNotificationConfig / wherever reading setConfigValue('notifications')) to
ignore/strip stored email_password and not map it back to
notifications.emailPassword so credentials remain env-only; use the existing
symbols setNotificationConfig, setConfigValue and the getter function name to
locate the changes.
In `@src/utils/config.ts`:
- Around line 43-47: The SMTP password resolution currently falls back to
getLegacyValue('email_password'), allowing plaintext persisted secrets to be
used; change the logic in the pass resolution (where
process.env.KDM_SMTP_PASSWORD is read) to remove the legacy fallback—require
process.env.KDM_SMTP_PASSWORD to be defined (or explicitly validate and
throw/log a clear error) and eliminate use of getLegacyValue('email_password')
so plaintext legacy secrets are no longer accepted at runtime.
- Around line 14-20: setConfig currently types Key as keyof
LegacyNotificationConfig which still allows forbidden keys like "email_password"
and only rejects them at runtime; define a compile-time union type (e.g.,
SensitiveLegacyKeyNames = 'email_password' | ... covering all entries in
sensitiveLegacyKeys) and change the generic to Key extends Exclude<keyof
LegacyNotificationConfig, SensitiveLegacyKeyNames> so callers cannot pass
forbidden keys; update or remove the runtime check that throws for
sensitiveLegacyKeys and adjust any call sites or tests that passed forbidden
keys to use environment variables instead; keep references to setConfig,
LegacyNotificationConfig, sensitiveLegacyKeys and the literal "email_password"
when making the change.
🪄 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: 5ab2bbe5-3e88-4b2e-ab0b-01d75d00c68c
📒 Files selected for processing (16)
src/__tests__/analysis.test.tssrc/__tests__/config-migration.test.tssrc/__tests__/config-store.test.tssrc/__tests__/config-utils.test.tssrc/__tests__/config.test.tssrc/analysis/analysis.tssrc/analysis/output.tssrc/analysis/stats.tssrc/analysis/types.tssrc/analyzers/index.tssrc/analyzers/types.tssrc/commands/config.tssrc/config/migration.tssrc/config/schema.tssrc/config/store.tssrc/utils/config.ts
Comment on lines
+33
to
+37
| describe('Analysis Engine', () => { | ||
| beforeEach(() => { | ||
| clearConfig(); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Reset analyzer registry state between tests to avoid cross-test pollution.
These tests mutate a shared singleton (registry.register(...)) but only clear config in beforeEach. That makes outcomes order-dependent across this file and other suites.
Suggested fix
describe('Analysis Engine', () => {
beforeEach(() => {
clearConfig();
+ // Ensure no analyzer registrations leak between tests.
+ // Use whichever API your registry exposes (e.g. clear/reset/unregisterAll).
+ registry.clear?.();
});As per coding guidelines, src/__tests__/**/*.ts requires: "Ensure mocks are clean and shared correctly."
Also applies to: 63-63, 110-111, 186-186
🤖 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__/analysis.test.ts` around lines 33 - 37, Tests are leaking a
shared analyzer registry state because only clearConfig() is called in
beforeEach; update the test setup to also reset the analyzer registry between
tests by calling the registry reset method (e.g., registry.clear() or
registry.reset()) in beforeEach (and/or afterEach) so any registry.register(...)
calls are undone; if no reset method exists, add a registry.reset/clear
implementation on the registry singleton and call that from the test beforeEach
blocks where registry.register is used (references: registry.register).
Comment on lines
+84
to
+119
| it('respects concurrency limit settings', async () => { | ||
| let activeCalls = 0; | ||
| let maxConcurrentCalls = 0; | ||
|
|
||
| const delayAnalyzer1 = { | ||
| name: 'Delay1', | ||
| analyze: async () => { | ||
| activeCalls++; | ||
| maxConcurrentCalls = Math.max(maxConcurrentCalls, activeCalls); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| activeCalls--; | ||
| return []; | ||
| }, | ||
| }; | ||
|
|
||
| const delayAnalyzer2 = { | ||
| name: 'Delay2', | ||
| analyze: async () => { | ||
| activeCalls++; | ||
| maxConcurrentCalls = Math.max(maxConcurrentCalls, activeCalls); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| activeCalls--; | ||
| return []; | ||
| }, | ||
| }; | ||
|
|
||
| registry.register(delayAnalyzer1); | ||
| registry.register(delayAnalyzer2); | ||
|
|
||
| await runAnalysis({ | ||
| filters: ['Delay1', 'Delay2'], | ||
| maxConcurrency: 1, | ||
| }); | ||
|
|
||
| expect(maxConcurrentCalls).toBe(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make the concurrency test deterministic instead of time-based.
Using real setTimeout(50) can be flaky/slower in CI. Prefer controlled promises or fake timers so concurrency assertions don’t depend on scheduler timing.
Suggested direction
- await new Promise((resolve) => setTimeout(resolve, 50));
+ await gate.promise; // manually release in test to control overlap deterministicallyAs per coding guidelines, src/__tests__/**/*.ts requires: "Ensure mocks are clean and shared correctly."
🤖 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__/analysis.test.ts` around lines 84 - 119, The test using real
setTimeouts in the 'respects concurrency limit settings' case is flaky; replace
the time-based delays with controlled promises so concurrency is deterministic:
create controllable promise resolvers for delayAnalyzer1 and delayAnalyzer2
(referenced as delayAnalyzer1, delayAnalyzer2) that increment/decrement
activeCalls inside their analyze methods and await their respective
externally-resolved promises, register them via registry.register(...), call
runAnalysis({ filters: ['Delay1','Delay2'], maxConcurrency: 1 }), then resolve
the promises in the desired order to assert maxConcurrentCalls === 1; ensure any
test-level mocks or fake timers are cleaned/reset per the project test
guidelines.
Comment on lines
+33
to
+47
| it('prefers explicit new notification config over migrated legacy values', () => { | ||
| const config = mergeLegacyConfig({ | ||
| notification_service: 'discord', | ||
| discord_webhook: 'legacy-webhook', | ||
| notifications: { | ||
| service: 'email', | ||
| discordWebhook: 'new-webhook', | ||
| }, | ||
| }); | ||
|
|
||
| expect(config.notifications).toMatchObject({ | ||
| service: 'email', | ||
| discordWebhook: 'new-webhook', | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add an explicit error-path test for mergeLegacyConfig.
This block validates precedence, but not malformed legacy input handling for mergeLegacyConfig itself. Add one test asserting safe fallback behavior (e.g., service: 'none') when legacy values are invalid.
As per coding guidelines, "Verify that tests cover both happy paths and error conditions."
🤖 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-migration.test.ts` around lines 33 - 47, Add a new unit
test in src/__tests__/config-migration.test.ts that covers the error path of
mergeLegacyConfig: call mergeLegacyConfig with malformed/invalid legacy fields
(e.g., notification_service set to an unexpected value or discord_webhook set to
a non-string) and assert the function safely falls back to the default
notifications object (for example notifications.service === 'none' and
notifications.discordWebhook undefined). Reference mergeLegacyConfig in the test
name and assertions so reviewers can find the handling for legacy -> new mapping
and validate the safe fallback behavior.
| }), | ||
| }; | ||
|
|
||
| (globalThis as any).mockConfigStore = mockConfigStore; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid leaking test mock state through globalThis.
Line 21 exposes mutable mock state globally and is unnecessary in this file. Keep the store closure-scoped to avoid cross-suite coupling/flakiness.
Proposed minimal fix
- (globalThis as any).mockConfigStore = mockConfigStore;As per coding guidelines, "Ensure mocks are clean and shared correctly."
📝 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
| (globalThis as any).mockConfigStore = mockConfigStore; |
🤖 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-store.test.ts` at line 21, Remove the global leak by
deleting the assignment "(globalThis as any).mockConfigStore = mockConfigStore"
and keep mockConfigStore as a closure-scoped test variable; if other suites need
access, expose it via a dedicated test helper or setup file (e.g., export a
getMockConfigStore helper or register it in jest.setup.ts) rather than writing
to globalThis so mock state stays isolated to the test scope.
Comment on lines
+85
to
+86
| const problems = results.reduce((acc, curr) => acc + curr.errors.length, 0); | ||
| const status = results.length > 0 ? 'ProblemDetected' : 'OK'; |
Contributor
There was a problem hiding this comment.
Align status with computed problem count.
At Line 86, status is derived from results.length, which can report ProblemDetected while Line 85 computes problems as 0. Use problems > 0 for consistent state.
Proposed fix
const problems = results.reduce((acc, curr) => acc + curr.errors.length, 0);
- const status = results.length > 0 ? 'ProblemDetected' : 'OK';
+ const status = problems > 0 ? 'ProblemDetected' : 'OK';🤖 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/analysis/analysis.ts` around lines 85 - 86, The status variable is
incorrectly based on results.length instead of the computed problems count,
causing mismatches; change the status assignment to use the already computed
problems (from results.reduce) so it becomes status = problems > 0 ?
'ProblemDetected' : 'OK' ensuring the declaration order keeps problems computed
before status (refer to the problems variable and the status assignment near
results.reduce).
Comment on lines
+48
to
+52
| for (const failure of res.errors) { | ||
| lines.push(` - ${chalk.red('Error:')} ${chalk.red(failure.text)}`); | ||
| if (failure.kubernetesDoc) { | ||
| lines.push(` ${chalk.red('Kubernetes Doc:')} ${chalk.red(failure.kubernetesDoc)}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Apply sensitive-value redaction before rendering failures.
Line 49 and Line 51 print raw strings. If failure.sensitive is populated, this can leak secrets to CLI output.
Proposed fix
export function formatTextOutput(output: AnalysisOutput): string {
const lines: string[] = [];
+ const redact = (value: string, sensitive?: { unmasked: string; masked: string }[]): string => {
+ if (!sensitive?.length) return value;
+ return sensitive.reduce(
+ (acc, pair) => (pair.unmasked ? acc.split(pair.unmasked).join(pair.masked) : acc),
+ value
+ );
+ };
@@
for (const failure of res.errors) {
- lines.push(` - ${chalk.red('Error:')} ${chalk.red(failure.text)}`);
+ lines.push(` - ${chalk.red('Error:')} ${chalk.red(redact(failure.text, failure.sensitive))}`);
if (failure.kubernetesDoc) {
- lines.push(` ${chalk.red('Kubernetes Doc:')} ${chalk.red(failure.kubernetesDoc)}`);
+ lines.push(
+ ` ${chalk.red('Kubernetes Doc:')} ${chalk.red(redact(failure.kubernetesDoc, failure.sensitive))}`
+ );
}
}🤖 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/analysis/output.ts` around lines 48 - 52, The loop that renders failures
prints raw values from failure.text and failure.kubernetesDoc which can leak
secrets when failure.sensitive is set; update the rendering in the for (const
failure of res.errors) block to pass those strings through a redaction helper
before pushing to lines (e.g., create or use a function named
redactSensitive(value: string, sensitive?: any) and call lines.push(` -
${chalk.red('Error:')} ${chalk.red(redactSensitive(failure.text,
failure.sensitive))}`) and similarly for failure.kubernetesDoc), or always run
redactSensitive on those fields to ensure sensitive data is redacted before
output.
Comment on lines
+46
to
+49
| notifications: { | ||
| ...legacyNotifications, | ||
| ...stored.notifications, | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid returning empty notifications objects without service.
Line 46 always emits a notifications object; when both sources are absent it becomes {}, so service is missing at runtime. Return undefined when no notification config exists (or default service explicitly).
Proposed fix
export const mergeLegacyConfig = (stored: StoredKDMConfig): KDMConfig => {
const legacyNotifications = notificationFromLegacy(stored);
+ const mergedNotifications =
+ legacyNotifications || stored.notifications
+ ? {
+ service: 'none' as const,
+ ...legacyNotifications,
+ ...stored.notifications,
+ }
+ : undefined;
return {
ai: stored.ai,
activeFilters: stored.activeFilters,
kubernetes: stored.kubernetes,
cache: stored.cache,
output: stored.output,
- notifications: {
- ...legacyNotifications,
- ...stored.notifications,
- },
+ notifications: mergedNotifications,
};
};📝 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
| notifications: { | |
| ...legacyNotifications, | |
| ...stored.notifications, | |
| }, | |
| export const mergeLegacyConfig = (stored: StoredKDMConfig): KDMConfig => { | |
| const legacyNotifications = notificationFromLegacy(stored); | |
| const mergedNotifications = | |
| legacyNotifications || stored.notifications | |
| ? { | |
| service: 'none' as const, | |
| ...legacyNotifications, | |
| ...stored.notifications, | |
| } | |
| : undefined; | |
| return { | |
| ai: stored.ai, | |
| activeFilters: stored.activeFilters, | |
| kubernetes: stored.kubernetes, | |
| cache: stored.cache, | |
| output: stored.output, | |
| notifications: mergedNotifications, | |
| }; | |
| }; |
🤖 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` around lines 46 - 49, The merged notifications
object created with "notifications: { ...legacyNotifications,
...stored.notifications }" can become an empty object and lack the required
"service" field; change the code to compute the mergedNotifications (using
legacyNotifications and stored.notifications) and only include the notifications
key when mergedNotifications has meaningful content (e.g.,
mergedNotifications.service is present or
Object.keys(mergedNotifications).length > 0), otherwise set notifications to
undefined (or explicitly provide a default service) so callers never receive an
empty {}.
Comment on lines
+68
to
+69
| export const setNotificationConfig = (notifications: NotificationConfig) => | ||
| setConfigValue('notifications', notifications); |
Contributor
There was a problem hiding this comment.
Block SMTP password persistence in config writes/reads.
Line 68 and Line 80 still allow storing/round-tripping SMTP password (notifications.emailPassword ↔ email_password). That reintroduces plaintext credential persistence, which conflicts with the env-only credential model.
Suggested direction
export const setNotificationConfig = (notifications: NotificationConfig) =>
- setConfigValue('notifications', notifications);
+ setConfigValue('notifications', {
+ ...notifications,
+ emailPassword: undefined,
+ });
export const getLegacyConfig = (): LegacyNotificationConfig => {
const notifications = getNotificationConfig();
return {
notification_service: notifications.service,
discord_webhook: notifications.discordWebhook,
email_host: notifications.emailHost,
email_port: notifications.emailPort,
email_user: notifications.emailUser,
email_to: notifications.emailTo,
- email_password: notifications.emailPassword,
+ // SMTP password must come from KDM_SMTP_PASSWORD env var only.
+ email_password: undefined,
alert_cooldown: notifications.alertCooldown,
};
};Also applies to: 71-82
🤖 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 68 - 69, The notifications SMTP password
must not be persisted or returned: update setNotificationConfig to remove/omit
notifications.emailPassword (and any email_password field) before calling
setConfigValue('notifications', ...), and update the corresponding getter (e.g.,
getNotificationConfig / wherever reading setConfigValue('notifications')) to
ignore/strip stored email_password and not map it back to
notifications.emailPassword so credentials remain env-only; use the existing
symbols setNotificationConfig, setConfigValue and the getter function name to
locate the changes.
Comment on lines
+14
to
+20
| export const setConfig = <Key extends keyof LegacyNotificationConfig>( | ||
| key: Key, | ||
| value: LegacyNotificationConfig[Key], | ||
| ) => { | ||
| if (sensitiveLegacyKeys.has(key)) { | ||
| throw new Error(`${key} must not be stored in config. Use the KDM_SMTP_PASSWORD environment variable instead.`); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Make forbidden config keys unrepresentable at compile time.
Line 14 currently allows email_password in the type system and rejects it only at runtime. Excluding it in the type signature makes key handling truly type-safe.
Suggested fix
-const sensitiveLegacyKeys = new Set<keyof LegacyNotificationConfig>(['email_password']);
+type WritableLegacyNotificationKey = Exclude<keyof LegacyNotificationConfig, 'email_password'>;
+const sensitiveLegacyKeys = new Set<keyof LegacyNotificationConfig>(['email_password']);
-export const setConfig = <Key extends keyof LegacyNotificationConfig>(
+export const setConfig = <Key extends WritableLegacyNotificationKey>(
key: Key,
value: LegacyNotificationConfig[Key],
) => {As per coding guidelines, “Ensure configuration keys are type-safe.”
🤖 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` around lines 14 - 20, setConfig currently types Key as
keyof LegacyNotificationConfig which still allows forbidden keys like
"email_password" and only rejects them at runtime; define a compile-time union
type (e.g., SensitiveLegacyKeyNames = 'email_password' | ... covering all
entries in sensitiveLegacyKeys) and change the generic to Key extends
Exclude<keyof LegacyNotificationConfig, SensitiveLegacyKeyNames> so callers
cannot pass forbidden keys; update or remove the runtime check that throws for
sensitiveLegacyKeys and adjust any call sites or tests that passed forbidden
keys to use environment variables instead; keep references to setConfig,
LegacyNotificationConfig, sensitiveLegacyKeys and the literal "email_password"
when making the change.
Comment on lines
43
to
47
| pass: | ||
| process.env.KDM_SMTP_PASSWORD !== undefined | ||
| ? process.env.KDM_SMTP_PASSWORD | ||
| : config.get('email_password'), | ||
| : getLegacyValue('email_password'), | ||
| }, |
Contributor
There was a problem hiding this comment.
Block stored SMTP password fallback in auth resolution.
Line 46 still falls back to getLegacyValue('email_password'), so a plaintext persisted secret can still be used at runtime even after setup removal.
Suggested fix
auth: {
user: getLegacyValue('email_user'),
- pass:
- process.env.KDM_SMTP_PASSWORD !== undefined
- ? process.env.KDM_SMTP_PASSWORD
- : getLegacyValue('email_password'),
+ pass: process.env.KDM_SMTP_PASSWORD,
},As per coding guidelines, “Verify that sensitive information is not stored in plain text if possible.”
📝 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
| pass: | |
| process.env.KDM_SMTP_PASSWORD !== undefined | |
| ? process.env.KDM_SMTP_PASSWORD | |
| : config.get('email_password'), | |
| : getLegacyValue('email_password'), | |
| }, | |
| auth: { | |
| user: getLegacyValue('email_user'), | |
| pass: process.env.KDM_SMTP_PASSWORD, | |
| }, |
🤖 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` around lines 43 - 47, The SMTP password resolution
currently falls back to getLegacyValue('email_password'), allowing plaintext
persisted secrets to be used; change the logic in the pass resolution (where
process.env.KDM_SMTP_PASSWORD is read) to remove the legacy fallback—require
process.env.KDM_SMTP_PASSWORD to be defined (or explicitly validate and
throw/log a clear error) and eliminate use of getLegacyValue('email_password')
so plaintext legacy secrets are no longer accepted at runtime.
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.
This PR implements Phase 2 of the Kubernetes Analyzer Engine for KDM. It ports the core analyzer registry, concurrent analysis execution engine, timing stats tracker, and output formatters from K8sGPT Go code to TypeScript.
Summary by CodeRabbit
New Features
Refactor
KDM_SMTP_PASSWORDenvironment variable for improved securityTests