Skip to content

fix: remove arguments from kdn log message - #1513

Merged
jeffmaury merged 1 commit into
openkaiden:mainfrom
jeffmaury:GH-1510
Apr 30, 2026
Merged

jeffmaury merged 1 commit into
openkaiden:mainfrom
jeffmaury:GH-1510

Conversation

@jeffmaury

Copy link
Copy Markdown
Contributor

Fixes #1510

Fixes openkaiden#1510

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury
jeffmaury requested a review from a team as a code owner April 29, 2026 20:10
@jeffmaury
jeffmaury requested review from MarsKubeX and fbricon and removed request for a team April 29, 2026 20:10
@coderabbitai

coderabbitai Bot commented Apr 29, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a5a10cbd-b148-49a5-8518-0d0e1b3f512a

📥 Commits

Reviewing files that changed from the base of the PR and between f6a3539 and 84373fe.

📒 Files selected for processing (1)
  • packages/main/src/plugin/kdn-cli/kdn-cli.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: linter, formatters
  • GitHub Check: typecheck
  • GitHub Check: Linux
  • GitHub Check: macOS
  • GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
  • GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
  • GitHub Check: unit tests / windows-2025
  • GitHub Check: Windows
  • GitHub Check: unit tests / macos-15
  • GitHub Check: unit tests / ubuntu-24.04
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use /@/ path aliases (e.g., '/@/plugin/provider-registry.js') instead of relative paths (e.g., '../plugin/provider-registry.js') for imports outside the current directory's module group. Relative imports are only used for sibling modules within the same directory.

Files:

  • packages/main/src/plugin/kdn-cli/kdn-cli.ts
packages/main/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

packages/main/src/**/*.{ts,tsx}: Long-running operations should use TaskManager with createTask() to provide user feedback on operation status
Container operations in the main process must use ContainerProviderRegistry with the engineId parameter to identify the container engine
Kubernetes operations in the main process should use KubernetesClient for context management, resource operations, port forwarding, and exec operations
IPC handlers in the main process must follow the naming convention: <registry-name>:<action> (e.g., container-provider-registry:listContainers)
Store credentials and sensitive setup data securely via SafeStorageRegistry instead of plain configuration

Files:

  • packages/main/src/plugin/kdn-cli/kdn-cli.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1417
File: extensions/kdn/src/kdn-extension.ts:44-52
Timestamp: 2026-04-23T04:29:13.867Z
Learning: In `extensions/kdn/src/kdn-extension.ts` (openkaiden/kaiden), when the `kdn` binary is found on the system PATH (the `findOnPath()` branch), `CliToolOptions.path` is intentionally set to the bare command name `'kdn'` (not `binaryName` like `'kdn.exe'`). This is because the absolute path is not known for system-managed installs, and Windows PATHEXT resolves the bare name correctly. This pattern is consistent with how other CLI tools register in the Kaiden codebase. Do not flag this as inconsistent or Windows-incorrect in future reviews.
📚 Learning: 2026-04-23T04:29:13.867Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1417
File: extensions/kdn/src/kdn-extension.ts:44-52
Timestamp: 2026-04-23T04:29:13.867Z
Learning: In `extensions/kdn/src/kdn-extension.ts` (openkaiden/kaiden), when the `kdn` binary is found on the system PATH (the `findOnPath()` branch), `CliToolOptions.path` is intentionally set to the bare command name `'kdn'` (not `binaryName` like `'kdn.exe'`). This is because the absolute path is not known for system-managed installs, and Windows PATHEXT resolves the bare name correctly. This pattern is consistent with how other CLI tools register in the Kaiden codebase. Do not flag this as inconsistent or Windows-incorrect in future reviews.

Applied to files:

  • packages/main/src/plugin/kdn-cli/kdn-cli.ts
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.

Applied to files:

  • packages/main/src/plugin/kdn-cli/kdn-cli.ts
🔇 Additional comments (1)
packages/main/src/plugin/kdn-cli/kdn-cli.ts (1)

145-145: Good hardening: sensitive CLI arguments are no longer logged on failure.

This directly addresses the secret exposure risk in createSecret failure paths while preserving the existing error-detail propagation behavior.


📝 Walkthrough

Walkthrough

A single-line change in the CLI error handler adjusts the error-logging path to prevent sensitive API key values from being exposed in console error messages when kdn secret create fails.

Changes

Cohort / File(s) Summary
Security Fix
packages/main/src/plugin/kdn-cli/kdn-cli.ts
Adjusted error-handling in execCLI to log only the resolved cliPath and extracted CLI error detail instead of full command arguments, preventing sensitive data (e.g., API keys) from being logged on command failure.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: removing arguments from the kdn log message to prevent sensitive information exposure.
Description check ✅ Passed The description references the linked issue #1510, which is directly related to the changeset addressing sensitive data logging.
Linked Issues check ✅ Passed The code change removes command arguments from the error log output in execCLI, directly addressing issue #1510's requirement to prevent API keys from being logged in plain text.
Out of Scope Changes check ✅ Passed The change is narrowly focused on the error-handling path in execCLI, directly addressing the scope of issue #1510 without introducing unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Apr 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@jeffmaury
jeffmaury merged commit ec7535f into openkaiden:main Apr 30, 2026
15 checks passed
@jeffmaury
jeffmaury deleted the GH-1510 branch April 30, 2026 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kdn secret create logs API key in plain text on failure

3 participants