fix(agent-workspace): create missing secret/provider during workspace creation - #2381
Conversation
… creation Secret/provider creation for inference connections can silently fail at registration time. When this happens, workspace creation proceeds without credentials. Now ensureModelSecretFromConfig creates the secret on the spot if the inference connection exists but its secret is missing. Fixes openkaiden#2371 Signed-off-by: Jeff MAURY <jmaury@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughOpenshellGateway now emits a start event during initialization. SecretManager listens for that event and refactors inference-secret creation behind new helper methods. AgentWorkspaceManager switches to the new secret lookup path and updates its tests accordingly. ChangesGateway start notifications and secret provisioning
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
Actionable comments posted: 2
🤖 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 `@packages/main/src/plugin/secret-manager/secret-manager.ts`:
- Around line 163-173: `createSecretForConnection` currently returns `undefined`
for both “secret already exists” and “cannot resolve secret type,” which makes
the result ambiguous. Update the existing-secret branch in
`SecretManager.createSecretForConnection` to return the already found
`SecretInfo` (or equivalent secret metadata) instead of `undefined`, while
keeping the missing-config path as-is so callers can distinguish no-op from
failure.
- Around line 98-106: `ensureSecretForModel` is doing redundant provider/secret
lookups by calling `getSecretForModel`, then
`providerRegistry.getInferenceConnection`, and finally
`createSecretForConnection` which triggers another `this.list()` call. Update
`SecretManager.ensureSecretForModel` to reuse the already-resolved
connection/secrets data from `getSecretForModel` instead of re-querying, and
adjust `getSecretForModel` and/or `createSecretForConnection` to accept or
return the pre-fetched info needed to avoid duplicate round-trips. Use the
`SecretManager`, `getSecretForModel`, `ensureSecretForModel`, and
`createSecretForConnection` paths to centralize the lookup and dedup logic.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3c02bfa-1331-4281-8208-8b300986775d
📒 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/secret-manager/secret-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: linter, formatters
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: unit tests / windows-2022
- GitHub Check: Linux
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: macOS
⚠️ CI failures not shown inline (4)
GitHub Actions: fullsend / dispatch _ Route: fix(agent-workspace): create missing secret/provider during workspace creation
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1m�[0m
�[36;1mif [[ ! "$STAGE" =~ ^[a-z][a-z0-9_-]*$ ]]; then�[0m
�[36;1m echo "::error::Invalid stage name: must start with lowercase letter and contain only [a-z0-9_-]"�[0m
GitHub Actions: fullsend / 6_dispatch _ Route.txt: fix(agent-workspace): create missing secret/provider during workspace creation
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1m�[0m
�[36;1mif [[ ! "$STAGE" =~ ^[a-z][a-z0-9_-]*$ ]]; then�[0m
�[36;1m echo "::error::Invalid stage name: must start with lowercase letter and contain only [a-z0-9_-]"�[0m
GitHub Actions: fullsend / dispatch _ Route: fix(agent-workspace): create missing secret/provider during workspace creation
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mEVENT_PAYLOAD=$(jq -c '{�[0m
�[36;1m issue: (.issue // null | if . then {number, html_url} else null end),�[0m
�[36;1m pull_request: (.pull_request // null | if . then {number, html_url,�[0m
�[36;1m head: {ref: .head.ref, sha: .head.sha, repo: {full_name: .head.repo.full_name}},�[0m
�[36;1m base: {ref: .base.ref, repo: {full_name: .base.repo.full_name}}} else null end),�[0m
�[36;1m comment: (.comment // null | if . then {body: .body[:4096]} else null end)�[0m
�[36;1m}' "$GITHUB_EVENT_PATH") || {�[0m
�[36;1m echo "::error::Failed to extract event payload from GITHUB_EVENT_PATH"�[0m
GitHub Actions: fullsend / dispatch _ Route: fix(agent-workspace): create missing secret/provider during workspace creation
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [[ -f .fullsend/config.yaml ]]; then�[0m
�[36;1m KILL_SWITCH=$(yq '.kill_switch // false' .fullsend/config.yaml)�[0m
�[36;1m if [[ "$KILL_SWITCH" == "true" ]]; then�[0m
�[36;1m echo "::error::Kill switch is active — all agent dispatch halted"�[0m
🧰 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/secret-manager/secret-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.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/secret-manager/secret-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.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/secret-manager/secret-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.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/secret-manager/secret-manager.spec.ts
🧠 Learnings (3)
📚 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/secret-manager/secret-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.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/secret-manager/secret-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.spec.ts
📚 Learning: 2026-06-29T13:16:53.102Z
Learnt from: benoitf
Repo: openkaiden/kaiden PR: 2296
File: extensions/container/packages/extension/src/helper/socket-finder/_socket-finder-module.ts:28-29
Timestamp: 2026-06-29T13:16:53.102Z
Learning: When reviewing imports in openkaiden/kaiden TypeScript/JavaScript files, prefer the configured `/@/` path alias instead of relative imports that would require traversing out of the current directory/module group (i.e., paths containing `..` that cross boundaries).
Do not require alias conversion for descendant-path relative imports within the socket-finder module directory—for example, in `extensions/container/packages/extension/src/helper/socket-finder/**`, imports like `./podman/podman-version-detector` and `./podman/podman-windows-finder` are acceptable and should not be flagged.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/secret-manager/secret-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/secret-manager/secret-manager.spec.ts
🔇 Additional comments (6)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
383-390: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
143-143: LGTM!Also applies to: 589-590, 633-633, 805-809, 818-818, 827-827
packages/main/src/plugin/secret-manager/secret-manager.spec.ts (2)
470-550: Test comment confirms the duplicate-lookup overhead flagged in the source file.The comment at Lines 499-500 explicitly documents
listProvidersbeing invoked twice perensureSecretForModelcall (once viagetSecretForModel, once via the dedup check increateSecretForConnection). This is good test coverage; see the companion comment onsecret-manager.tsLines 98-106 for the suggested consolidation.
380-468: LGTM!packages/main/src/plugin/secret-manager/secret-manager.ts (2)
175-177: LGTM!
108-173: 🩺 Stability & AvailabilitySerialize secret creation per connection.
onDidRegisterInferenceConnectionandensureSecretForModelcan both reach this non-atomiclist()→create()path for the samesecretName, so concurrent calls can race into a duplicate create. If the CLI rejects duplicate provider names, an in-flight map keyed bysecretNameor an idempotent create path would avoid the failure.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
MarsKubeX
left a comment
MarsKubeX
left a comment
There was a problem hiding this comment.
LGTM. Tested and working as expected
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
2 participants
How to test
Summary
SecretManager.onInferenceConnectionRegisteredinto a publiccreateSecretForConnectionmethod, and addedensureSecretForModelwhich creates the secret on the spot if the inference connection exists but its secret is missing.AgentWorkspaceManager.ensureModelSecretFromConfignow callsensureSecretForModelinstead ofgetSecretForModel, so workspaces always get their credentials when a connection is available.Fixes #2371
Test plan
createSecretForConnection— creates secret when none exists, returns undefined when_typenot configured, returns undefined when secret already existsensureSecretForModel— returns existing secret without creating, creates and returns secret when missing but connection exists, returns undefined when no inference connection foundAgentWorkspaceManagertests to useensureSecretForModel🤖 Generated with Claude Code