fix: redact secret values from kdn CLI error logs - #1949
serbangeorge-m merged 2 commits into
Conversation
When `kdn secret create` fails (e.g. duplicate entry), the error handler in execCLI logged the full command args including `--value` with the plaintext API key. Add redactSensitiveArgs() to replace sensitive flag values with `***` before logging. Closes openkaiden#1944 Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: serbangeorge-m <serbangeorge.m@gmail.com>
Move the "already exists" handling from ensureModelSecret into
KdnCli.createSecret so the error is caught before execCLI logs it.
When a secret already exists, createSecret now returns { name }
silently instead of logging a console.error and forcing callers
to catch-and-ignore.
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: serbangeorge-m <serbangeorge.m@gmail.com>
📝 WalkthroughWalkthroughThis PR addresses bug ChangesKdnCli Secret Redaction and Duplicate Handling
AgentWorkspaceManager Secret Creation Simplification
Sequence DiagramsequenceDiagram
participant AM as AgentWorkspaceManager
participant KdnCli
participant exec as exec.exec
AM->>KdnCli: createSecret(name, value, ...)
activate KdnCli
KdnCli->>exec: exec secret create --output json
alt Secret created successfully
exec-->>KdnCli: { name: "..." }
KdnCli-->>AM: { name: "..." }
else Secret already exists
exec-->>KdnCli: error with "already exists"
KdnCli-->>AM: { name: options.name }
else Other error
exec-->>KdnCli: error
KdnCli->>KdnCli: redactSensitiveArgs()
KdnCli-->>AM: throw (after logging redacted command)
end
deactivate KdnCli
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/main/src/plugin/kdn-cli/kdn-cli.ts (1)
337-375:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse case-insensitive matching for the "already exists" error message.
The test suite (lines 942 and 954 of kdn-cli.spec.ts) confirms the error format as
'secret "my-secret": secret already exists', but the substring check on line 368 is case-sensitive. If the kdn CLI outputs this message with different casing (e.g., "Already exists" or "ALREADY EXISTS"), the graceful handling will fail and throw an error instead. Update the check todetail.toLowerCase().includes('already exists')to be more resilient.🤖 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 `@packages/main/src/plugin/kdn-cli/kdn-cli.ts` around lines 337 - 375, In createSecret, the check for the CLI "already exists" message is case-sensitive and can miss variants; update the conditional that examines the extracted error detail (from extractCliError) to perform a case-insensitive match (e.g. use detail.toLowerCase().includes('already exists')) so that the method still returns { name: options.name } when the CLI reports an existing secret regardless of casing; keep the existing redaction and logging behavior for other errors.
🤖 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.
Outside diff comments:
In `@packages/main/src/plugin/kdn-cli/kdn-cli.ts`:
- Around line 337-375: In createSecret, the check for the CLI "already exists"
message is case-sensitive and can miss variants; update the conditional that
examines the extracted error detail (from extractCliError) to perform a
case-insensitive match (e.g. use detail.toLowerCase().includes('already
exists')) so that the method still returns { name: options.name } when the CLI
reports an existing secret regardless of casing; keep the existing redaction and
logging behavior for other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85ef880a-805d-4587-9adc-cef89130ce10
📒 Files selected for processing (4)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.ts
📜 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: unit tests / windows-2025
- GitHub Check: Linux
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.ts
🧠 Learnings (2)
📚 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/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/kdn-cli/kdn-cli.tspackages/main/src/plugin/kdn-cli/kdn-cli.spec.ts
🪛 OpenGrep (1.21.0)
packages/main/src/plugin/kdn-cli/kdn-cli.ts
[ERROR] 364-364: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (6)
packages/main/src/plugin/kdn-cli/kdn-cli.ts (2)
307-321: LGTM!
331-332: LGTM!packages/main/src/plugin/kdn-cli/kdn-cli.spec.ts (2)
871-881: LGTM!Also applies to: 890-890, 898-898, 906-906, 925-925
939-985: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
152-152: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
643-655: LGTM!Also applies to: 657-667
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
fbricon
left a comment
fbricon
left a comment
There was a problem hiding this comment.
I would probably make redactSensitiveArgs more generic and pass the sensitive flags as args, and have a redactSecretArgs function wrap it for ['--value'], but that's unnecessary for now as it's only used for secrets, so maybe overkill for now. We can always refactor it if/when needed.
2 participants
Summary
--value(API key/secret) fromconsole.erroroutput whenkdn secret createfails, replacing it with***KdnCli.createSecret— returns{ name }instead of logging an error and forcing callers to catch-and-ignoreAgentWorkspaceManager.ensureModelSecretby removing its now-unnecessary try/catch for duplicate secretsTest plan
returns secret name silently when secret already exists,does not log error when secret already exists,rethrows non-"already exists" errors,redacts secret value from error logs on non-"already exists" failurekdn-cli.spec.tstests passagent-workspace-manager.spec.tstests passsecret-manager.spec.tstests passCloses #1944
Made with Cursor