feat(vertex-ai): route Vertex AI through standard openshell provider pipeline#2252
Open
jeffmaury wants to merge 1 commit into
Open
feat(vertex-ai): route Vertex AI through standard openshell provider pipeline#2252jeffmaury wants to merge 1 commit into
jeffmaury wants to merge 1 commit into
Conversation
…pipeline Add flags support to CreateProviderOptions and SecretValue interfaces, allowing bare positional arguments to be passed to `openshell provider create`. When a `_flag` configuration property is detected, non-password config values are returned as environment variables for the workspace instead of being passed as provider config. This enables Vertex AI to use the standard `ensureModelSecretFromConfig` path with ADC authentication. Changes: - Add `flags` field to CreateProviderOptions and SecretValue - Handle bare flag arguments in OpenshellCli.createProvider - Forward config and flags in OpenshellSecretAdapter.createSecret - Return environment variables from ensureModelSecret/ensureModelSecretFromConfig - Accept empty credentials when flags are provided Fixes openkaiden#2245 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jeff MAURY <jmaury@redhat.com>
jeffmaury
requested review from
benoitf and
fbricon
and removed request for
a team
Contributor
📝 WalkthroughWalkthroughAdds an optional ChangesVertex AI flags support in provider/secret pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
Contributor
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/agent-workspace/agent-workspace-manager.ts (1)
251-292: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winEnvironment variables are computed but never applied to the workspace configuration.
The function now returns
Promise<Record<string, string>>, but the caller at line 125 doesn't capture the return value:await this.ensureModelSecret(options);When the config-based path at lines 257-258 returns
result.environment, those environment variables are lost. They should be applied tooptions.workspaceConfiguration.environment(similar to the pattern at lines 282-289) before returning.Additionally, the try-finally block at lines 257-261 has an empty finally clause that serves no purpose and should be removed.
🔧 Proposed fix to apply environment variables
try { const result = await this.ensureModelSecretFromConfig(options); - if (result.handled) return result.environment; - } finally { - /* empty */ - } + if (result.handled) { + // Apply environment variables to workspace configuration + if (Object.keys(result.environment).length > 0) { + options.workspaceConfiguration ??= {}; + options.workspaceConfiguration.environment ??= []; + for (const [name, value] of Object.entries(result.environment)) { + options.workspaceConfiguration.environment = options.workspaceConfiguration.environment.filter( + e => e.name !== name, + ); + options.workspaceConfiguration.environment.push({ name, value }); + } + } + return {}; + } + } catch { + // Fall through to legacy path + }🤖 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/agent-workspace/agent-workspace-manager.ts` around lines 251 - 292, The ensureModelSecret method returns environment variables from the config-based path at lines 257-258 but those variables are never applied to the workspace configuration and are lost. When result.environment is returned from ensureModelSecretFromConfig, apply those environment variables to options.workspaceConfiguration.environment before returning, following the same pattern used at lines 282-289 where result.environmentVariable is pushed to the environment array. Additionally, remove the empty try-finally block at lines 257-261 as it serves no purpose.
🤖 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/agent-workspace/agent-workspace-manager.ts`:
- Around line 251-292: The ensureModelSecret method returns environment
variables from the config-based path at lines 257-258 but those variables are
never applied to the workspace configuration and are lost. When
result.environment is returned from ensureModelSecretFromConfig, apply those
environment variables to options.workspaceConfiguration.environment before
returning, following the same pattern used at lines 282-289 where
result.environmentVariable is pushed to the environment array. Additionally,
remove the empty try-finally block at lines 257-261 as it serves no purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e69dd9e-eeec-40a0-ba3c-e645a45042df
📒 Files selected for processing (8)
packages/api/src/openshell-gateway-info.tspackages/api/src/secret-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2022
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: typecheck
- GitHub Check: Linux
🧰 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/api/src/secret-info.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-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/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🧠 Learnings (2)
📚 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/api/src/secret-info.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.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/secret-manager/openshell-secret-adapter.tspackages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🔇 Additional comments (12)
packages/api/src/openshell-gateway-info.ts (1)
128-128: LGTM!packages/api/src/secret-info.ts (1)
39-39: LGTM!packages/main/src/plugin/openshell-cli/openshell-cli.ts (2)
283-283: LGTM!
292-296: LGTM!packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
766-766: LGTM!Also applies to: 778-827
packages/main/src/plugin/secret-manager/openshell-secret-adapter.ts (1)
56-57: LGTM!packages/main/src/plugin/secret-manager/openshell-secret-adapter.spec.ts (1)
58-59: LGTM!Also applies to: 69-93
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (4)
290-300: LGTM!
315-319: LGTM!
340-369: LGTM!
381-381: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
1138-1214: LGTM!
fbricon
reviewed
Jun 23, 2026
| } | ||
|
|
||
| const flagEntry = connectionProperties.find(([fullKey]) => fullKey.endsWith('._flag')); | ||
| const flagValue = flagEntry ? config.get<string>(flagEntry[0]) : undefined; |
Contributor
There was a problem hiding this comment.
I'm confused as flags are defined as string[] yet only the 1st flag is being used
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
fbricon
reviewed
Jun 23, 2026
| 'google-vertex-ai', | ||
| '--credential', | ||
| 'GOOGLE_APPLICATION_CREDENTIALS', | ||
| '__from_gcloud_adc', |
Contributor
There was a problem hiding this comment.
openshell doc mentions --from-gcloud-adc, why can't we use that instead of __from_gcloud_adc?
benoitf
approved these changes
Jun 23, 2026
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
3 participants
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
flagssupport toCreateProviderOptionsandSecretValueinterfaces, enabling bare positional arguments foropenshell provider create_flagconfiguration property is detected, non-password config values are returned as workspace environment variables instead of provider config — this lets Vertex AI use ADC authentication through the standardensureModelSecretFromConfigpathconfigandflagsthroughOpenshellSecretAdapter.createSecret(fixes a pre-existing gap whereconfigwas defined onSecretValuebut never forwarded)Test plan
OpenshellCli.createProviderwith bare flag argumentsOpenshellSecretAdapterforwarding config and flagsensureModelSecretFromConfigreturning environment variables when_flagis setFixes #2245
🤖 Generated with Claude Code