feat(onecli): configure secrets for OneCLI credential gateway #278
Merged
fbricon merged 4 commits intoopenkaiden:mainfrom Apr 22, 2026
Merged
feat(onecli): configure secrets for OneCLI credential gateway #278fbricon merged 4 commits intoopenkaiden:mainfrom
fbricon merged 4 commits intoopenkaiden:mainfrom
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR implements OneCLI-based secret provisioning for workspace instances. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/Manager
participant Manager as Instance Manager
participant Mapper as Secret Mapper
participant Runtime as Podman Runtime
participant Provisioner as Secret Provisioner
participant CredProvider as Credential Provider
participant OnecliClient as OneCLI Client
User->>Manager: Add workspace with secrets
Manager->>Mapper: Map workspace secrets
Mapper-->>Manager: CreateSecretInput[]
Manager->>Runtime: Create pod with OnecliSecrets
Runtime->>Runtime: setupOnecli()
Runtime->>Runtime: Start postgres container
Runtime->>CredProvider: Get API key
CredProvider->>OnecliClient: Request /api/user/api-key
OnecliClient-->>CredProvider: API key
CredProvider-->>Runtime: oc_key
Runtime->>OnecliClient: Initialize with baseURL & apiKey
Runtime->>Provisioner: Provision secrets
Provisioner->>OnecliClient: CreateSecret(input)
OnecliClient-->>Provisioner: Secret created or 409 Conflict
alt Conflict
Provisioner->>OnecliClient: ListSecrets()
OnecliClient-->>Provisioner: []Secret
Provisioner->>OnecliClient: UpdateSecret(id, input)
OnecliClient-->>Provisioner: success
end
Provisioner-->>Runtime: Secrets provisioned
Runtime->>OnecliClient: GetContainerConfig()
OnecliClient-->>Runtime: ContainerConfig{Env, CACertificate}
Runtime->>Runtime: Create workspace container with proxy env vars
Runtime->>Runtime: Mount CA certificate
Runtime-->>Manager: Pod ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/runtime/podman/create.go (1)
300-334:⚠️ Potential issue | 🟠 MajorOrphaned pod on OneCLI setup failure.
kube play --start=falseat line 303 creates the pod;setupOneclithen starts thepostgresandoneclicontainers and only stops the pod on the happy path (line 430). Every earlyreturninsetupOnecli(postgres start/readiness, onecli start, readiness wait, API key retrieval, secret provisioning, container-config fetch) leaves the pod and any started containers running in podman. The subsequentCreatecall for the same workspace name will then fail because the pod already exists, forcing the user into manualpodman pod rm -fcleanup.Recommend registering a cleanup (
pod stop+kube play --down/pod rm -f) that runs wheneversetupOneclifails, or at the very least whenCreatereturns an error afterkube playsucceeded.🧹 Sketch of a best-effort cleanup on the failure path
if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(), "kube", "play", "--start=false", tmpYAMLPath); err != nil { stepLogger.Fail(err) return runtime.RuntimeInfo{}, fmt.Errorf("failed to create pod via kube play: %w", err) } + + // Ensure the pod is torn down if any subsequent step fails before the + // workspace container is successfully created. + podCreated := true + defer func() { + if !podCreated { + return + } + // Best-effort teardown; swallow errors so we don't mask the original failure. + _ = p.executor.Run(ctx, io.Discard, io.Discard, "pod", "rm", "-f", params.Name) + }()…and clear
podCreated = falseon the successful return path (afterwritePodFiles). Adjust to match your existing cleanup conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create.go` around lines 300 - 334, The pod created by p.executor.Run("kube","play","--start=false", tmpYAMLPath) can be left running if setupOnecli returns early, causing orphaned pods; add a best-effort cleanup that runs when setupOnecli or Create fails: after the successful kube play call, set a local podCreated flag and install a deferred cleanup that checks the flag and, on failure, runs pod stop/remove (via p.executor.Run with "pod", "stop" / "pod", "rm","-f" or "kube","play","--down") to tear down the pod and any started containers; ensure you clear podCreated (or flip a success boolean) on the successful path (e.g., after writePodFiles) so the defer is a no-op on success, and reference setupOnecli, p.executor.Run, tmpYAMLPath, podCreated and writePodFiles when locating where to add the defer.
🧹 Nitpick comments (5)
pkg/runtime/podman/create_test.go (1)
413-611: LGTM.Call-site updates consistently thread
nilfor the new*containerConfigArgsparameter across allbuildContainerArgssubtests. Consider adding a follow-up subtest that passes a non-nilcontainerConfigArgsto assert OneCLI-derived env vars and CA-cert mount end up in the container args — not required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create_test.go` around lines 413 - 611, Tests currently always pass nil for the new *containerConfigArgs to buildContainerArgs; add one subtest that constructs a non-nil containerConfigArgs (populate fields that represent OneCLI-derived env vars and the CA cert mount) and call p.buildContainerArgs(params, imageName, cfg) to assert the returned args include the expected env entries and the CA certificate volume mount; reference buildContainerArgs and the containerConfigArgs struct to locate where to create and pass the config and what fields to check.pkg/runtime/podman/podman_test.go (1)
182-347: Tests correctly track removal of postgres/proxy host port fields.Optional: since the host ports for postgres (5432) and the OneCLI proxy (10255) are no longer exposed to the host, you could add negative assertions (e.g.,
!strings.Contains(yamlStr, "containerPort: 5432")/"containerPort: 10255") inTestRenderPodYAMLto catch accidental re-exposure of those ports on the host in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/podman_test.go` around lines 182 - 347, Add negative assertions in the TestRenderPodYAML test to ensure renderPodYAML does not re-expose postgres or proxy ports: after calling renderPodYAML(data) in the "renders all template fields" subtest, assert that the returned YAML string does NOT contain "containerPort: 5432" and does NOT contain "containerPort: 10255" (use the existing yamlStr variable and strings.Contains checks), so that renderPodYAML and podTemplateData changes will fail the test if those host-facing port lines reappear.pkg/onecli/provisioner_test.go (2)
89-122: Coverage gap: partial-failure paths for multi-secret input.
fakeClient.erris a single static error, so the current tests can't verify that:
- After a 409 on secret N, iteration continues and secret N+1 is still attempted/created.
- On a non-409 error at secret N (> 0), the returned message correctly includes index N and name.
Consider adding a per-call error function on
fakeClient(e.g.,errFunc func(CreateSecretInput) error) to exercise these paths and assert the index/name in the wrapped error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/provisioner_test.go` around lines 89 - 122, The tests need per-call error control: extend the fakeClient used by NewSecretProvisioner with a new field errFunc func(CreateSecretInput) error (in addition to or replacing err) and make its CreateSecret method call errFunc(input) if set; then update or add tests to use errFunc to return a 409 for the first secret and nil for subsequent calls to assert iteration continues (verify both secrets were attempted) and to return a non-409 error for a later secret to assert ProvisionSecrets returns the wrapped error containing the failing index and secret Name; target symbols: fakeClient, err/errFunc, CreateSecretInput, CreateSecret, ProvisionSecrets, NewSecretProvisioner.
28-51: Add a compile-time interface check forfakeClient.Prevents silent drift if the
Clientinterface evolves (e.g., a new method is added).♻️ Proposed addition
type fakeClient struct { created []CreateSecretInput err error } + +var _ Client = (*fakeClient)(nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/provisioner_test.go` around lines 28 - 51, Add a compile-time interface assertion to ensure fakeClient implements the Client interface: declare a var or _ assignment using the Client type and fakeClient (e.g., var _ Client = (*fakeClient)(nil)). Place this near the fakeClient type definition so the compiler will fail if the Client interface changes and fakeClient no longer satisfies it; reference the fakeClient type and the Client interface in the assertion.pkg/runtime/podman/start.go (1)
91-121: Consider usingcontext.WithTimeoutinstead of a parallel deadline variable.Minor cleanup:
waitForReadymaintains its owndeadlinewithtime.Now().Before(deadline)while also threadingctx. Usingcontext.WithTimeout(ctx, readinessTimeout)would unify both into a single cancellation signal, simplify the loop, and guarantee the per-request context honors the overall budget.♻️ Proposed refactor
func waitForReady(ctx context.Context, baseURL string) error { - deadline := time.Now().Add(readinessTimeout) - httpClient := &http.Client{Timeout: 5 * time.Second} - - for time.Now().Before(deadline) { - if ctx.Err() != nil { - return ctx.Err() - } - - req, err := http.NewRequestWithContext(ctx, http.MethodGet, baseURL+"/api/health", nil) + ctx, cancel := context.WithTimeout(ctx, readinessTimeout) + defer cancel() + httpClient := &http.Client{Timeout: 5 * time.Second} + + for { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, baseURL+"/api/health", nil) if err != nil { return err } resp, err := httpClient.Do(req) if err == nil { resp.Body.Close() if resp.StatusCode < 500 { return nil } } select { case <-ctx.Done(): - return ctx.Err() + return fmt.Errorf("timed out waiting for OneCLI at %s: %w", baseURL, ctx.Err()) case <-time.After(readinessInterval): } } - - return fmt.Errorf("timed out after %s waiting for OneCLI at %s", readinessTimeout, baseURL) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/start.go` around lines 91 - 121, Refactor waitForReady to use context.WithTimeout(ctx, readinessTimeout) instead of the manual deadline: create a derived ctx (e.g., ctx, cancel := context.WithTimeout(ctx, readinessTimeout)) and defer cancel, remove the deadline/time.Now().Before loop condition, and drive the loop off ctx.Done() and time.After(readinessInterval); ensure each HTTP request uses the derived context via http.NewRequestWithContext(ctx, ...) and update the final error to reflect the timeout when the derived context expires.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/onecli/mapper.go`:
- Around line 108-113: The code silently drops extra hosts because
otherHostPattern(hosts *[]string) returns only the first element; add explicit
validation where secrets are mapped (e.g., in Map) to detect when secret.Hosts
has len>1 and either return a clear error or emit a warning log directing users
to declare one secret per host until the API supports multiple host patterns;
keep otherHostPattern as the single-pattern helper but ensure Map performs the
check and surfaces the message (include the function names Map and
otherHostPattern in the change so reviewers can find the new validation).
In `@pkg/onecli/provisioner.go`:
- Around line 46-56: Update the godoc for secretProvisioner.ProvisionSecrets to
explicitly state that pre-existing secrets are left unchanged on 409 Conflict
and that value updates are not applied by this method (they require
UI/delete+recreate or other out-of-band action). In the ProvisionSecrets
function, when errors.As(err, &apiErr) && apiErr.StatusCode ==
http.StatusConflict, emit a debug-level log noting the suppressed 409 (including
the secret name and index) instead of silently continuing; if secretProvisioner
has no logger field, add a lightweight logger (e.g., p.logger with Debugf) or
use the existing package logger to record the message. Ensure the log references
the secret name (input.Name) and index (i) so operators can trace ignored
updates.
In `@pkg/runtime/podman/create.go`:
- Around line 156-164: The code in podman create.go appends OneCLI proxy env
vars from ccArgs after workspace env vars (envVars), causing OneCLI values to
override user WorkspaceConfig.Environment; update the logic in the block that
handles ccArgs (the env var loop using ccArgs.envVars and the CA mount logic) so
that user-defined workspace env vars take precedence—either by prepending
ccArgs.envVars only when keys do not exist in the workspace env map or by
appending workspace vars after ccArgs.envVars; alternatively, if current
precedence is intentional, add a clear processLogger warning when a key
collision is detected between ccArgs.envVars and the workspace env map
(reference ccArgs, ccArgs.envVars, and the args slice in create.go).
- Around line 415-426: The provisioning and container-config steps start with
stepLogger.Start but don't call stepLogger.Fail on their error returns; update
the error paths in the block that calls onecli.NewSecretProvisioner and
provisioner.ProvisionSecrets (the "Provisioning OneCLI secrets" step) and the
block that calls client.GetContainerConfig (the "Retrieving OneCLI container
config" step) to call stepLogger.Fail(err) immediately before returning the
wrapped fmt.Errorf, mirroring the pattern used for earlier steps (e.g., OneCLI
start/readiness); keep the existing Start messages and ensure each early return
invokes stepLogger.Fail with the captured error.
In `@pkg/runtime/podman/pods/onecli-pod.yaml`:
- Line 8: The pod spec currently sets restartPolicy: Never which prevents
automatic restarts for transient crashes of postgres or onecli; change
restartPolicy to OnFailure to allow the kubelet to restart failed containers
(or, if Never is intentional, add a short YAML comment next to restartPolicy
explaining the rationale and why auto-restart must be avoided), and ensure this
is applied in the pod spec that defines the onecli/postgres sidecars so future
reviewers see the intended behavior.
In `@pkg/runtime/podman/start.go`:
- Around line 62-73: Start currently launches the onecli container in Start but
doesn't wait for it to be ready; call the existing waitForReady helper after the
p.executor.Run that starts onecli (i.e., right after starting onecliContainer
and before starting the workspace container) to block until OneCLI /api/health
responds. Ensure the OneCLI web port value is available to waitForReady by
reading it from podTemplateData (or the rendered YAML) the same way setupOnecli
does during creation, and pass that port (and pod name) into waitForReady so
Start mirrors the readiness behavior used in setupOnecli/create.go.
---
Outside diff comments:
In `@pkg/runtime/podman/create.go`:
- Around line 300-334: The pod created by
p.executor.Run("kube","play","--start=false", tmpYAMLPath) can be left running
if setupOnecli returns early, causing orphaned pods; add a best-effort cleanup
that runs when setupOnecli or Create fails: after the successful kube play call,
set a local podCreated flag and install a deferred cleanup that checks the flag
and, on failure, runs pod stop/remove (via p.executor.Run with "pod", "stop" /
"pod", "rm","-f" or "kube","play","--down") to tear down the pod and any started
containers; ensure you clear podCreated (or flip a success boolean) on the
successful path (e.g., after writePodFiles) so the defer is a no-op on success,
and reference setupOnecli, p.executor.Run, tmpYAMLPath, podCreated and
writePodFiles when locating where to add the defer.
---
Nitpick comments:
In `@pkg/onecli/provisioner_test.go`:
- Around line 89-122: The tests need per-call error control: extend the
fakeClient used by NewSecretProvisioner with a new field errFunc
func(CreateSecretInput) error (in addition to or replacing err) and make its
CreateSecret method call errFunc(input) if set; then update or add tests to use
errFunc to return a 409 for the first secret and nil for subsequent calls to
assert iteration continues (verify both secrets were attempted) and to return a
non-409 error for a later secret to assert ProvisionSecrets returns the wrapped
error containing the failing index and secret Name; target symbols: fakeClient,
err/errFunc, CreateSecretInput, CreateSecret, ProvisionSecrets,
NewSecretProvisioner.
- Around line 28-51: Add a compile-time interface assertion to ensure fakeClient
implements the Client interface: declare a var or _ assignment using the Client
type and fakeClient (e.g., var _ Client = (*fakeClient)(nil)). Place this near
the fakeClient type definition so the compiler will fail if the Client interface
changes and fakeClient no longer satisfies it; reference the fakeClient type and
the Client interface in the assertion.
In `@pkg/runtime/podman/create_test.go`:
- Around line 413-611: Tests currently always pass nil for the new
*containerConfigArgs to buildContainerArgs; add one subtest that constructs a
non-nil containerConfigArgs (populate fields that represent OneCLI-derived env
vars and the CA cert mount) and call p.buildContainerArgs(params, imageName,
cfg) to assert the returned args include the expected env entries and the CA
certificate volume mount; reference buildContainerArgs and the
containerConfigArgs struct to locate where to create and pass the config and
what fields to check.
In `@pkg/runtime/podman/podman_test.go`:
- Around line 182-347: Add negative assertions in the TestRenderPodYAML test to
ensure renderPodYAML does not re-expose postgres or proxy ports: after calling
renderPodYAML(data) in the "renders all template fields" subtest, assert that
the returned YAML string does NOT contain "containerPort: 5432" and does NOT
contain "containerPort: 10255" (use the existing yamlStr variable and
strings.Contains checks), so that renderPodYAML and podTemplateData changes will
fail the test if those host-facing port lines reappear.
In `@pkg/runtime/podman/start.go`:
- Around line 91-121: Refactor waitForReady to use context.WithTimeout(ctx,
readinessTimeout) instead of the manual deadline: create a derived ctx (e.g.,
ctx, cancel := context.WithTimeout(ctx, readinessTimeout)) and defer cancel,
remove the deadline/time.Now().Before loop condition, and drive the loop off
ctx.Done() and time.After(readinessInterval); ensure each HTTP request uses the
derived context via http.NewRequestWithContext(ctx, ...) and update the final
error to reflect the timeout when the derived context expires.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20db0842-dbf1-4fd5-8abb-886980af2e8f
📒 Files selected for processing (18)
pkg/instances/manager.gopkg/onecli/client.gopkg/onecli/client_test.gopkg/onecli/credentials.gopkg/onecli/credentials_test.gopkg/onecli/mapper.gopkg/onecli/mapper_test.gopkg/onecli/provisioner.gopkg/onecli/provisioner_test.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/podman.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/pods/onecli-pod.yamlpkg/runtime/podman/start.gopkg/runtime/podman/start_test.gopkg/runtime/podman/steplogger_test.gopkg/runtime/runtime.go
Comment on lines
+46
to
+56
| func (p *secretProvisioner) ProvisionSecrets(ctx context.Context, secrets []CreateSecretInput) error { | ||
| for i, input := range secrets { | ||
| if _, err := p.client.CreateSecret(ctx, input); err != nil { | ||
| var apiErr *APIError | ||
| if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusConflict { | ||
| continue | ||
| } | ||
| return fmt.Errorf("failed to create secret %d (%q): %w", i, input.Name, err) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Silent 409 handling may mask stale secret values.
Treating 409 Conflict as a no-op means that if a user updates a secret's value in their config after the secret was first provisioned, the new value will never be applied on subsequent workspace creations — the existing (stale) secret simply remains. This is likely acceptable for the initial feature (and matches the PR's manual test flow of editing via the OneCLI web UI), but it's an important contract to capture.
Recommended:
- Expand the godoc on
ProvisionSecretsto explicitly state that a pre-existing secret is left unchanged (not updated) and that value updates require out-of-band action (UI/delete+recreate). - Consider at least logging at debug level when a 409 is suppressed, so operators can see that a value update was ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/onecli/provisioner.go` around lines 46 - 56, Update the godoc for
secretProvisioner.ProvisionSecrets to explicitly state that pre-existing secrets
are left unchanged on 409 Conflict and that value updates are not applied by
this method (they require UI/delete+recreate or other out-of-band action). In
the ProvisionSecrets function, when errors.As(err, &apiErr) && apiErr.StatusCode
== http.StatusConflict, emit a debug-level log noting the suppressed 409
(including the secret name and index) instead of silently continuing; if
secretProvisioner has no logger field, add a lightweight logger (e.g., p.logger
with Debugf) or use the existing package logger to record the message. Ensure
the log references the secret name (input.Name) and index (i) so operators can
trace ignored updates.
Comment on lines
+154
to
+166
| // readPodTemplateData reads the persisted pod template data for a container. | ||
| func (p *podmanRuntime) readPodTemplateData(containerID string) (podTemplateData, error) { | ||
| path := filepath.Join(p.podDir(containerID), podTemplateDataFile) | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return podTemplateData{}, fmt.Errorf("failed to read pod template data: %w", err) | ||
| } | ||
| var tmplData podTemplateData | ||
| if err := json.Unmarshal(data, &tmplData); err != nil { | ||
| return podTemplateData{}, fmt.Errorf("failed to unmarshal pod template data: %w", err) | ||
| } | ||
| return tmplData, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -C2 '\breadPodTemplateData\s*\('Repository: openkaiden/kdn
Length of output: 491
🏁 Script executed:
# Check the full context around readPodTemplateData and writePodFiles
rg -nP --type=go -B5 -A15 'func \(p \*podmanRuntime\) readPodTemplateData'Repository: openkaiden/kdn
Length of output: 1582
🏁 Script executed:
# Also search for writePodFiles to understand the persistence mechanism
rg -nP --type=go -B2 -A20 'func \(p \*podmanRuntime\) writePodFiles'Repository: openkaiden/kdn
Length of output: 1745
🏁 Script executed:
# Check if there are any TODOs or FIXMEs that mention readPodTemplateData or podTemplateData usage
rg -nP --type=go '(TODO|FIXME|XXX).*[Pp]odTemplateData|[Pp]odTemplateData.*(TODO|FIXME|XXX)'Repository: openkaiden/kdn
Length of output: 40
Remove unused readPodTemplateData method or add the missing call site.
The method is confirmed unused by golangci-lint and has no callers in the codebase. However, writePodFiles does persist template data (including OnecliWebPort and OnecliVersion) to a JSON file, suggesting the read operation was intended. Either remove the unused method to eliminate dead code, or add the expected call site(s) in Start/Create flows where the persisted data should be retrieved.
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 155-155: func (*podmanRuntime).readPodTemplateData is unused
(unused)
fbricon
force-pushed
the
worktree-GH-266
branch
2 times, most recently
from
d63d907 to
8f06b0c
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/runtime/runtime.go (1)
25-25: Runtime package now depends onpkg/onecli.Adding
onecli.CreateSecretInputto the genericCreateParamscouples the runtime abstraction to a specific provisioning concept. This is acceptable if OneCLI is meant to be first-class across runtimes, but if you want other runtimes (docker, process, fake) to remain OneCLI-agnostic, consider introducing a runtime-neutral type (e.g.,SecretSpecdefined inpkg/runtime) and translating it inside the podman runtime. Non-blocking.Also applies to: 77-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/runtime.go` at line 25, The runtime package has become coupled to pkg/onecli by using onecli.CreateSecretInput inside CreateParams; to decouple, define a runtime-neutral SecretSpec (or similar) type in pkg/runtime and replace onecli.CreateSecretInput usage in CreateParams with this new SecretSpec, then update the podman/runtime implementation to translate SecretSpec -> onecli.CreateSecretInput before calling OneCLI; ensure all other runtimes (docker, process, fake) consume the runtime.SecretSpec so they remain OneCLI-agnostic and remove the direct import of pkg/onecli from pkg/runtime.pkg/onecli/credentials.go (2)
56-57: Consider reusing a singlehttp.Client.A new
http.Client(and therefore a fresh transport/connection pool) is allocated on everyAPIKey()call. For a one-shot bootstrap this is fine, but ifAPIKeyis invoked multiple times, hoist the client intocredentialProvideras a struct field configured inNewCredentialProvider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/credentials.go` around lines 56 - 57, The APIKey method currently allocates a new http.Client per call which recreates transports/connection pools; add an httpClient *http.Client field to the credentialProvider struct, initialize it (Timeout: 10*time.Second and any other needed settings) inside NewCredentialProvider, and update credentialProvider.APIKey to reuse p.httpClient instead of creating a new client on every invocation; remove the per-call allocation to preserve connection reuse and pooling.
82-84: EmptyapiKeyresponse produces a slightly misleading error.If the server returns
{"apiKey": ""}(or the field is missing), the error surfaces as"API key has unexpected format", which could be confusing. Consider distinguishing the empty/missing case explicitly.Suggested change
+ if result.APIKey == "" { + return "", fmt.Errorf("API key missing in response") + } if !strings.HasPrefix(result.APIKey, apiKeyPrefix) { return "", fmt.Errorf("API key has unexpected format") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/credentials.go` around lines 82 - 84, Check for an empty or missing API key before validating its prefix: in the code path that inspects result.APIKey (the variable used with apiKeyPrefix), if result.APIKey is empty or nil return a clear error like "API key missing or empty" instead of the generic format error, and only then perform the strings.HasPrefix(result.APIKey, apiKeyPrefix) check to return the "unexpected format" error when the value is present but malformed.pkg/onecli/client.go (2)
158-211: Consider preserving status code inAPIError.Error()output.
APIError.Error()returns onlyMessage, so wrapped errors lose the HTTP status code in logs (e.g., a 409 and a 500 can look identical if the server doesn't return anerrorfield). Including the status code would make diagnostics easier.Suggested change
func (e *APIError) Error() string { - return e.Message + return fmt.Sprintf("onecli API error (status %d): %s", e.StatusCode, e.Message) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/client.go` around lines 158 - 211, The APIError type's Error() currently returns only the Message, losing the HTTP status code when wrapped; update APIError.Error() to include the StatusCode (e.g., prefix or suffix the message with the numeric code and optionally http.StatusText) so callers and logs of client.do can see both status and message; locate the APIError type and its Error() method and change its return string to include StatusCode (and human-readable text), and ensure any tests or callers expecting the old string are adjusted if needed.
168-168: Trailing slash inbaseURLproduces double-slash paths.
c.baseURL + pathwill yieldhttp://host//api/secretsif callers pass abaseURLwith a trailing/. Consider trimming or usingurl.JoinPathfor robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/client.go` at line 168, The request URL construction using c.baseURL + path can produce double slashes when c.baseURL ends with '/', so update the request creation in the function that builds req (where http.NewRequestWithContext is called) to join base and path safely—either trim trailing slashes from c.baseURL (e.g., via strings.TrimRight) or use url.JoinPath / url.Parse + ResolveReference to construct the full URL before passing it to http.NewRequestWithContext; ensure you reference and replace the current concatenation of c.baseURL + path so all callers benefit.pkg/onecli/credentials_test.go (1)
81-89:http://localhost:1is not a portable way to get an unreachable address.On some CI environments port 1 behavior differs (connection refused vs. hang until timeout, depending on OS/firewall). A deterministic approach is to start an
httptest.NewServer, immediatelyClose()it, and reuse its URL — guaranteeing connection refusal on a port that was just released.Suggested change
- provider := NewCredentialProvider("http://localhost:1") - _, err := provider.APIKey(context.Background()) + server := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + server.Close() // force a closed port URL + provider := NewCredentialProvider(server.URL) + _, err := provider.APIKey(context.Background())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/onecli/credentials_test.go` around lines 81 - 89, Replace the hardcoded "http://localhost:1" in TestCredentialProvider_Unreachable with a deterministically unreachable URL by creating an httptest.NewServer(), capturing its URL, immediately calling Close() and passing that URL to NewCredentialProvider; keep the rest of the test using provider.APIKey(ctx) and asserting an error. This change should be made inside the TestCredentialProvider_Unreachable test to ensure connection refusal across CI environments.pkg/runtime/podman/start_test.go (1)
34-51: Helper's*httptest.Serverreturn value is always discarded.All four call sites (lines 100, 161, 183, 276) ignore the server with
_. Sincet.Cleanup(srv.Close)already handles teardown, consider dropping the second return value to simplify the helper's signature.♻️ Proposed simplification
-func startHealthServer(t *testing.T) (int, *httptest.Server) { +func startHealthServer(t *testing.T) int { t.Helper() ... - port := listener.Addr().(*net.TCPAddr).Port - return port, srv + return listener.Addr().(*net.TCPAddr).Port }And update callers:
-healthPort, _ := startHealthServer(t) +healthPort := startHealthServer(t)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/start_test.go` around lines 34 - 51, The helper startHealthServer currently returns (int, *httptest.Server) but the *httptest.Server is always ignored by callers and teardown is already handled via t.Cleanup(srv.Close); change startHealthServer signature to return only int, remove the server from the return, keep creating srv and t.Cleanup(srv.Close) inside the function, and update all callers (the startHealthServer(...) invocations) to accept just the port value instead of a two-value assignment; ensure references to srv in the helper remain for setup/cleanup but do not return it.pkg/runtime/podman/create.go (1)
161-170: Warning bypasses logger and writes directly toos.Stderr.The rest of this file uses
logger.FromContext(ctx)(see line 309) for stdout/stderr. Writing the collision warning directly toos.Stderrhere will bypass any logger wiring (prefixes, redirection in tests, structured output). SincebuildContainerArgsdoesn't currently accept acontext.Context, consider threading one through (or accepting alogger.Logger) so the warning goes through the same channel as other runtime output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create.go` around lines 161 - 170, The warning is written directly to os.Stderr inside buildContainerArgs, bypassing the project's logger; update buildContainerArgs to accept a context.Context (or a logger.Logger) and replace fmt.Fprintf(os.Stderr, ...) with logger.FromContext(ctx).Warnf("OneCLI overrides workspace env var %q", k) (or the equivalent logger method), and propagate the new ctx parameter through any callers (e.g., where buildContainerArgs is invoked) so the warning uses the same logging pipeline and test redirection as the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/onecli/client_test.go`:
- Line 55: The test calls json.NewEncoder(w).Encode(want) without checking the
returned error; update these calls in pkg/onecli/client_test.go (the occurrences
that call json.NewEncoder(w).Encode — at the shown line and the other
occurrences around lines 83 and 106) to explicitly handle or ignore the error by
assigning it (e.g., _ = json.NewEncoder(w).Encode(want)) or by checking and
failing the test on error; ensure you modify each Encode call in the test helper
or test functions so golangci-lint errcheck is satisfied.
In `@pkg/runtime/podman/create.go`:
- Around line 315-349: The deferred cleanup using p.executor.Run currently
reuses the caller's ctx (via podCreatedOK defer) so if Create's ctx is cancelled
the pod rm will likely no-op; change the defer to capture a fresh
non-cancellable/short‑lived context (e.g., create ctxCleanup :=
context.Background() or context.WithTimeout(context.Background(), shortTimeout)
and cancel appropriately) and call p.executor.Run(ctxCleanup, l.Stdout(),
l.Stderr(), "pod", "rm", "-f", params.Name) inside the defer that checks
podCreatedOK so cleanup runs even when the original ctx is cancelled; update
references around podCreatedOK and the existing defer block in Create to use the
new ctx variable.
- Around line 334-347: The CA cert written under
p.storageDir/certs/<podName>/onecli-ca.pem is never removed; update Remove()
(which calls cleanupPodFiles()) to also remove the certs directory for the pod:
compute certsDir := filepath.Join(p.storageDir, "certs", <podName or params.Name
used when creating the pod>) and call os.RemoveAll(certsDir) (log any non-fatal
errors rather than failing removal) so certificates don't accumulate; reference
the existing cleanupPodFiles(), Remove(), and p.storageDir symbols when making
the change.
---
Nitpick comments:
In `@pkg/onecli/client.go`:
- Around line 158-211: The APIError type's Error() currently returns only the
Message, losing the HTTP status code when wrapped; update APIError.Error() to
include the StatusCode (e.g., prefix or suffix the message with the numeric code
and optionally http.StatusText) so callers and logs of client.do can see both
status and message; locate the APIError type and its Error() method and change
its return string to include StatusCode (and human-readable text), and ensure
any tests or callers expecting the old string are adjusted if needed.
- Line 168: The request URL construction using c.baseURL + path can produce
double slashes when c.baseURL ends with '/', so update the request creation in
the function that builds req (where http.NewRequestWithContext is called) to
join base and path safely—either trim trailing slashes from c.baseURL (e.g., via
strings.TrimRight) or use url.JoinPath / url.Parse + ResolveReference to
construct the full URL before passing it to http.NewRequestWithContext; ensure
you reference and replace the current concatenation of c.baseURL + path so all
callers benefit.
In `@pkg/onecli/credentials_test.go`:
- Around line 81-89: Replace the hardcoded "http://localhost:1" in
TestCredentialProvider_Unreachable with a deterministically unreachable URL by
creating an httptest.NewServer(), capturing its URL, immediately calling Close()
and passing that URL to NewCredentialProvider; keep the rest of the test using
provider.APIKey(ctx) and asserting an error. This change should be made inside
the TestCredentialProvider_Unreachable test to ensure connection refusal across
CI environments.
In `@pkg/onecli/credentials.go`:
- Around line 56-57: The APIKey method currently allocates a new http.Client per
call which recreates transports/connection pools; add an httpClient *http.Client
field to the credentialProvider struct, initialize it (Timeout: 10*time.Second
and any other needed settings) inside NewCredentialProvider, and update
credentialProvider.APIKey to reuse p.httpClient instead of creating a new client
on every invocation; remove the per-call allocation to preserve connection reuse
and pooling.
- Around line 82-84: Check for an empty or missing API key before validating its
prefix: in the code path that inspects result.APIKey (the variable used with
apiKeyPrefix), if result.APIKey is empty or nil return a clear error like "API
key missing or empty" instead of the generic format error, and only then perform
the strings.HasPrefix(result.APIKey, apiKeyPrefix) check to return the
"unexpected format" error when the value is present but malformed.
In `@pkg/runtime/podman/create.go`:
- Around line 161-170: The warning is written directly to os.Stderr inside
buildContainerArgs, bypassing the project's logger; update buildContainerArgs to
accept a context.Context (or a logger.Logger) and replace fmt.Fprintf(os.Stderr,
...) with logger.FromContext(ctx).Warnf("OneCLI overrides workspace env var %q",
k) (or the equivalent logger method), and propagate the new ctx parameter
through any callers (e.g., where buildContainerArgs is invoked) so the warning
uses the same logging pipeline and test redirection as the rest of the file.
In `@pkg/runtime/podman/start_test.go`:
- Around line 34-51: The helper startHealthServer currently returns (int,
*httptest.Server) but the *httptest.Server is always ignored by callers and
teardown is already handled via t.Cleanup(srv.Close); change startHealthServer
signature to return only int, remove the server from the return, keep creating
srv and t.Cleanup(srv.Close) inside the function, and update all callers (the
startHealthServer(...) invocations) to accept just the port value instead of a
two-value assignment; ensure references to srv in the helper remain for
setup/cleanup but do not return it.
In `@pkg/runtime/runtime.go`:
- Line 25: The runtime package has become coupled to pkg/onecli by using
onecli.CreateSecretInput inside CreateParams; to decouple, define a
runtime-neutral SecretSpec (or similar) type in pkg/runtime and replace
onecli.CreateSecretInput usage in CreateParams with this new SecretSpec, then
update the podman/runtime implementation to translate SecretSpec ->
onecli.CreateSecretInput before calling OneCLI; ensure all other runtimes
(docker, process, fake) consume the runtime.SecretSpec so they remain
OneCLI-agnostic and remove the direct import of pkg/onecli from pkg/runtime.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a68385ff-0c47-41e8-99ca-3074487bb6ad
📒 Files selected for processing (19)
pkg/cmd/workspace_start.gopkg/instances/manager.gopkg/onecli/client.gopkg/onecli/client_test.gopkg/onecli/credentials.gopkg/onecli/credentials_test.gopkg/onecli/mapper.gopkg/onecli/mapper_test.gopkg/onecli/provisioner.gopkg/onecli/provisioner_test.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/podman.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/pods/onecli-pod.yamlpkg/runtime/podman/start.gopkg/runtime/podman/start_test.gopkg/runtime/podman/steplogger_test.gopkg/runtime/runtime.go
✅ Files skipped from review due to trivial changes (1)
- pkg/instances/manager.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/runtime/podman/podman_test.go
- pkg/runtime/podman/create_test.go
- pkg/onecli/provisioner_test.go
- pkg/runtime/podman/podman.go
- pkg/onecli/provisioner.go
- pkg/runtime/podman/start.go
- pkg/onecli/mapper.go
Comment on lines
+334
to
+347
| // Write CA certificate to a durable location for mounting into the workspace container. | ||
| // Use a shared certs directory under storageDir (not instanceDir which is cleaned up). | ||
| if containerConfig.CACertificate != "" && containerConfig.CACertificateContainerPath != "" { | ||
| certsDir := filepath.Join(p.storageDir, "certs", params.Name) | ||
| if mkErr := os.MkdirAll(certsDir, 0755); mkErr != nil { | ||
| return runtime.RuntimeInfo{}, fmt.Errorf("failed to create certs directory: %w", mkErr) | ||
| } | ||
| caPath := filepath.Join(certsDir, "onecli-ca.pem") | ||
| if writeErr := os.WriteFile(caPath, []byte(containerConfig.CACertificate), 0644); writeErr != nil { | ||
| return runtime.RuntimeInfo{}, fmt.Errorf("failed to write CA certificate: %w", writeErr) | ||
| } | ||
| ccArgs.caFilePath = caPath | ||
| ccArgs.caContainerPath = containerConfig.CACertificateContainerPath | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Delete/teardown logic in podman runtime and check for certs cleanup
fd -e go . pkg/runtime/podman | xargs rg -nP -C3 '\b(Delete|Remove|cleanup|teardown)\b'
rg -nP -C3 '"certs"' pkg/runtime/podmanRepository: openkaiden/kdn
Length of output: 10722
🏁 Script executed:
# Read the full Remove() implementation in remove.go
cat -n pkg/runtime/podman/remove.goRepository: openkaiden/kdn
Length of output: 3181
🏁 Script executed:
# Find the cleanupPodFiles implementation
fd -e go . pkg/runtime/podman | xargs rg -nP 'func.*cleanupPodFiles'Repository: openkaiden/kdn
Length of output: 152
🏁 Script executed:
# Look for any cleanup of certs directory
rg -nP 'storageDir.*certs|certs.*storageDir' pkg/runtime/podmanRepository: openkaiden/kdn
Length of output: 280
🏁 Script executed:
# Read the cleanupPodFiles implementation
sed -n '140,180p' pkg/runtime/podman/podman.goRepository: openkaiden/kdn
Length of output: 1492
🏁 Script executed:
# Find the podDir implementation
rg -nP 'func.*podDir|podDir\s*:=' pkg/runtime/podman/podman.goRepository: openkaiden/kdn
Length of output: 120
🏁 Script executed:
# Read the podDir implementation
sed -n '99,110p' pkg/runtime/podman/podman.goRepository: openkaiden/kdn
Length of output: 579
Add CA certificate cleanup to the Remove() function.
The CA certificate is written to p.storageDir/certs/<podName>/onecli-ca.pem but cleanupPodFiles() only removes p.storageDir/pods/<containerID>. The certs directory is not cleaned up during workspace deletion, causing certificate files to accumulate over the lifecycle of workspaces. Update Remove() to also remove the corresponding certs directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create.go` around lines 334 - 347, The CA cert written
under p.storageDir/certs/<podName>/onecli-ca.pem is never removed; update
Remove() (which calls cleanupPodFiles()) to also remove the certs directory for
the pod: compute certsDir := filepath.Join(p.storageDir, "certs", <podName or
params.Name used when creating the pod>) and call os.RemoveAll(certsDir) (log
any non-fatal errors rather than failing removal) so certificates don't
accumulate; reference the existing cleanupPodFiles(), Remove(), and p.storageDir
symbols when making the change.
jeffmaury
reviewed
Apr 17, 2026
Contributor
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
When using a defined secret service (github), the environment variables are not injected into the agent container (thus I think the onecli layer should only be visible in the runtime level)
Also this PR mixes several concerns: implementing secret and removing exposed ports
fbricon
force-pushed
the
worktree-GH-266
branch
2 times, most recently
from
3c97d60 to
3c4e09e
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/runtime/podman/remove.go (1)
43-45:⚠️ Potential issue | 🟡 MinorClean cert artifacts when the container is already gone.
If Podman reports the container as not found, this returns after
cleanupPodFiles(id)and skips the new cert cleanup. SincecleanupPodFilescan remove the saved pod name, resolve it first and clean certs before returning.Proposed fix
if isNotFoundError(err) { + if podName, readErr := p.readPodName(id); readErr == nil { + p.cleanupCerts(podName) + } p.cleanupPodFiles(id) return nil }Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/remove.go` around lines 43 - 45, When isNotFoundError(err) is true, the code currently calls p.cleanupPodFiles(id) and returns before running the certificate cleanup; change the branch so you first resolve the pod name (use the existing resolver used elsewhere, e.g. p.resolvePodName(id) or the same lookup used later in the function) then invoke the cert-cleanup logic (the same certificate cleanup call used later in this function) for that resolved pod name, and only afterward call p.cleanupPodFiles(id) and return; apply the same change to the other isNotFoundError branch later in the function.pkg/runtime/podman/podman.go (1)
1-1:⚠️ Potential issue | 🟡 MinorRun
make fmtbefore merging.CI reports this Go file is not gofmt-formatted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/podman.go` at line 1, This file (pkg/runtime/podman/podman.go, top-level podman.go header/package) is not gofmt-formatted; run the project's formatting step (make fmt) or run gofmt/gofiles tools locally to reformat podman.go so it matches CI expectations, then stage and commit the resulting changes; ensure any import ordering (goimports) is applied if the repository expects it.pkg/runtime/podman/create.go (1)
1-1:⚠️ Potential issue | 🟡 MinorRun
make fmtbefore merging.CI reports this Go file is not gofmt-formatted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create.go` at line 1, The file create.go in the podman runtime has formatting issues reported by CI; run the project's formatter (make fmt) or gofmt -w on create.go to apply gofmt formatting, then stage and commit the formatted file so CI passes and the file matches gofmt style.
♻️ Duplicate comments (2)
pkg/runtime/podman/start.go (1)
69-84:⚠️ Potential issue | 🟠 MajorWait for OneCLI readiness before
Startreturns.
pod startcan return before the OneCLI gateway accepts requests, so callers may enter a workspace whose proxy is not ready yet. The existingwaitForReadyhelper can use the persisted pod template data to wait onOnecliWebPort.Proposed fix
// Start the rest of the pod (onecli + workspace); already-running // containers (postgres) are left untouched by pod start. stepLogger.Start(fmt.Sprintf("Starting pod: %s", podName), "Pod started") if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(), "pod", "start", podName); err != nil { stepLogger.Fail(err) return runtime.RuntimeInfo{}, fmt.Errorf("failed to start pod: %w", err) } + tmplData, err := p.readPodTemplateData(id) + if err != nil { + stepLogger.Fail(err) + return runtime.RuntimeInfo{}, fmt.Errorf("failed to read pod template data: %w", err) + } + stepLogger.Start("Waiting for OneCLI readiness", "OneCLI ready") + if err := waitForReady(ctx, fmt.Sprintf("http://localhost:%d", tmplData.OnecliWebPort)); err != nil { + stepLogger.Fail(err) + return runtime.RuntimeInfo{}, fmt.Errorf("OneCLI service not ready: %w", err) + } + // Install OneCLI CA certificate into system trust store if present if caPath := p.readCAContainerPath(id); caPath != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/start.go` around lines 69 - 84, The Start flow returns immediately after p.executor.Run("pod", "start", podName) but must wait until OneCLI is accepting requests; after successful pod start (and before installing CA cert or returning) call the existing waitForReady helper using the persisted pod template data to wait on the OnecliWebPort for readiness (use pod identifier id/podName to load the template), surface errors through stepLogger.Fail and wrap them like other failures, and only proceed to p.installCACert and return runtime info once waitForReady confirms OneCLI is ready.pkg/runtime/podman/podman.go (1)
173-173:⚠️ Potential issue | 🟡 MinorWire up or remove the unused pod-template reader.
golangci-lintreportsreadPodTemplateDataas unused. Use it from the OneCLI readiness path inStart, or remove it if the metadata read is not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/podman.go` at line 173, The function readPodTemplateData is currently unused; either call it from the OneCLI readiness path inside Start or remove it. To wire it up: locate the OneCLI readiness sequence in Start and invoke p.readPodTemplateData(containerID) where containerID is the container you already inspect for readiness (handle and log any error and use the returned podTemplateData as needed). If the metadata is not needed, delete the readPodTemplateData function and remove any now-unused imports or tests referencing it; ensure Start’s readiness flow compiles and behavior remains unchanged after the removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/onecli/client.go`:
- Around line 42-89: The structs Exported from pkg (CreateSecretInput,
UpdateSecretInput, InjectionConfig, Secret, ContainerConfig, APIError) violate
the pkg public-type rule; fix by making these DTOs unexported (rename to
createSecretInput, updateSecretInput, injectionConfig, secret, containerConfig,
apiError) and expose only exported interfaces or factory/accessor functions in
this package (e.g., NewCreateSecretInput, CreateSecretInputDTO interface, or
GetSecret accessor) or alternatively move the concrete JSON DTOs into an allowed
API package and keep only interfaces in pkg; ensure any compile-time interface
assertions remain (unnamed var _: Interface = (*concrete)(nil)) and update
usages of functions referencing CreateSecretInput, UpdateSecretInput,
InjectionConfig, Secret, ContainerConfig, and APIError accordingly.
- Line 184: Replace the bare defer resp.Body.Close() with an explicit discard of
the Close error so errcheck is satisfied; after calling io.ReadAll on resp.Body,
use a deferred closure like defer func() { _ = resp.Body.Close() }() (or check
and assign cerr := resp.Body.Close(); _ = cerr) to explicitly ignore the error
from resp.Body.Close().
In `@pkg/onecli/credentials_test.go`:
- Around line 32-37: Test HTTP handlers in credentials_test.go are ignoring
errors from json.NewEncoder(w).Encode (and any w.Write) which trips errcheck;
update each httptest.NewServer handler closure (the anonymous http.HandlerFunc
where json.NewEncoder(w).Encode(...) is called at the shown locations and the
other occurrences around lines 53-56 and 69-71) to capture the error return and
fail the test on error (e.g., if err := json.NewEncoder(w).Encode(...); err !=
nil { t.Fatalf("encode response failed: %v", err) }) or similarly handle write
errors from w.Write, so all Encode/Write calls return-checked and tests surface
failures.
In `@pkg/onecli/credentials.go`:
- Line 68: Replace the bare deferred call to resp.Body.Close() with an explicit
discard of its return value to satisfy errcheck: change the defer
resp.Body.Close() to a defer func() { _ = resp.Body.Close() }() (optionally
drain the body with io.Copy(io.Discard, resp.Body) before closing if needed).
Update the code in the function that uses resp (the place where
resp.Body.Close() is currently deferred) so the close error is explicitly
ignored by assigning it to the blank identifier.
In `@pkg/runtime/podman/create_test.go`:
- Around line 583-584: The test currently writes the CA fixture with
os.WriteFile(caFile, []byte("cert-data"), 0644) but ignores its error; update
the test to check the returned error (e.g., err := os.WriteFile(...); if err !=
nil { t.Fatalf("failed to write CA fixture: %v", err) }) so the test fails
immediately on write problems and doesn't proceed with a missing CA file when
buildContainerArgs runs.
- Around line 654-670: Initialize the sentinel indices onecliIdx and wsIdx to -1
(not 0), then after the loop that scans args verify both were found (i.e.
onecliIdx != -1 and wsIdx != -1) and fail the test with a clear message if
either is missing; only then assert ordering (onecliIdx > wsIdx) to ensure a
missing env var cannot incorrectly satisfy the precedence check — update the
variables and add the presence checks around the existing loop in the test that
inspects args.
In `@pkg/runtime/podman/create.go`:
- Around line 358-362: When buildContainerArgs fails after calling
stepLogger.Start, mark the step as failed before returning: in the function
where stepLogger.Start("Creating workspace container: %s", params.Name) is
called and buildContainerArgs(params, imageName, ccArgs) returns an error, call
stepLogger.Fail(err) (including the error) and then return the
runtime.RuntimeInfo and error; update the error path that currently just returns
to ensure stepLogger.Fail(err) is invoked so the workspace-container step is
recorded as failed.
In `@pkg/runtime/podman/remove.go`:
- Around line 88-90: The cleanupCerts function currently calls
os.RemoveAll(filepath.Join(p.storageDir, "certs", podName)) without sanitizing
podName or returning errors; update cleanupCerts on podmanRuntime to first clean
and validate the joined path (use filepath.Clean and ensure the resulting path
has p.storageDir/certs as its prefix to prevent path traversal), call
os.RemoveAll on the validated path, and return the error instead of ignoring it
so callers can handle failure; reference cleanupCerts, podName, p.storageDir,
and os.RemoveAll in your changes.
In `@pkg/runtime/podman/start.go`:
- Around line 149-154: The current readiness check treats any status <500 as
success (e.g., 401/404); update the check after httpClient.Do(req) so it only
returns success for a genuine successful health response (e.g., require
resp.StatusCode >= 200 && resp.StatusCode < 300 or another explicit success
range you prefer) instead of resp.StatusCode < 500; keep the resp.Body.Close()
behavior and ensure the function continues retrying for non-success codes like
401/404.
- Around line 99-108: installCACert currently interpolates caContainerPath into
a sh -c string which risks shell injection; change it to build the destination
path using path.Join for the container trust dir and invoke two separate
p.executor.Run exec calls (one for "cp" with args: containerID, caContainerPath,
joined destination path; and one for "update-ca-trust" with args: containerID,
"update-ca-trust") instead of a single "sh -c" call so no shell interpolation is
used and each command is executed directly.
---
Outside diff comments:
In `@pkg/runtime/podman/create.go`:
- Line 1: The file create.go in the podman runtime has formatting issues
reported by CI; run the project's formatter (make fmt) or gofmt -w on create.go
to apply gofmt formatting, then stage and commit the formatted file so CI passes
and the file matches gofmt style.
In `@pkg/runtime/podman/podman.go`:
- Line 1: This file (pkg/runtime/podman/podman.go, top-level podman.go
header/package) is not gofmt-formatted; run the project's formatting step (make
fmt) or run gofmt/gofiles tools locally to reformat podman.go so it matches CI
expectations, then stage and commit the resulting changes; ensure any import
ordering (goimports) is applied if the repository expects it.
In `@pkg/runtime/podman/remove.go`:
- Around line 43-45: When isNotFoundError(err) is true, the code currently calls
p.cleanupPodFiles(id) and returns before running the certificate cleanup; change
the branch so you first resolve the pod name (use the existing resolver used
elsewhere, e.g. p.resolvePodName(id) or the same lookup used later in the
function) then invoke the cert-cleanup logic (the same certificate cleanup call
used later in this function) for that resolved pod name, and only afterward call
p.cleanupPodFiles(id) and return; apply the same change to the other
isNotFoundError branch later in the function.
---
Duplicate comments:
In `@pkg/runtime/podman/podman.go`:
- Line 173: The function readPodTemplateData is currently unused; either call it
from the OneCLI readiness path inside Start or remove it. To wire it up: locate
the OneCLI readiness sequence in Start and invoke
p.readPodTemplateData(containerID) where containerID is the container you
already inspect for readiness (handle and log any error and use the returned
podTemplateData as needed). If the metadata is not needed, delete the
readPodTemplateData function and remove any now-unused imports or tests
referencing it; ensure Start’s readiness flow compiles and behavior remains
unchanged after the removal.
In `@pkg/runtime/podman/start.go`:
- Around line 69-84: The Start flow returns immediately after
p.executor.Run("pod", "start", podName) but must wait until OneCLI is accepting
requests; after successful pod start (and before installing CA cert or
returning) call the existing waitForReady helper using the persisted pod
template data to wait on the OnecliWebPort for readiness (use pod identifier
id/podName to load the template), surface errors through stepLogger.Fail and
wrap them like other failures, and only proceed to p.installCACert and return
runtime info once waitForReady confirms OneCLI is ready.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a1dbf24-8df0-4877-a296-31a69f682b83
📒 Files selected for processing (23)
pkg/cmd/service_list_test.gopkg/cmd/workspace_start.gopkg/instances/manager.gopkg/onecli/client.gopkg/onecli/client_test.gopkg/onecli/credentials.gopkg/onecli/credentials_test.gopkg/onecli/mapper.gopkg/onecli/mapper_test.gopkg/onecli/provisioner.gopkg/onecli/provisioner_test.gopkg/runtime/podman/config/defaults.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/podman.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/pods/onecli-pod.yamlpkg/runtime/podman/remove.gopkg/runtime/podman/start.gopkg/runtime/podman/steplogger_test.gopkg/runtime/runtime.gopkg/secretservicesetup/register_test.gopkg/secretservicesetup/secretservices.json
✅ Files skipped from review due to trivial changes (3)
- pkg/runtime/podman/config/defaults.go
- pkg/secretservicesetup/register_test.go
- pkg/runtime/podman/pods/onecli-pod.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/cmd/workspace_start.go
- pkg/instances/manager.go
- pkg/onecli/client_test.go
- pkg/onecli/mapper.go
- pkg/onecli/provisioner.go
- pkg/onecli/provisioner_test.go
Comment on lines
+42
to
+89
| // UpdateSecretInput is the request body for updating a secret. | ||
| type UpdateSecretInput struct { | ||
| Value *string `json:"value,omitempty"` | ||
| HostPattern *string `json:"hostPattern,omitempty"` | ||
| PathPattern *string `json:"pathPattern,omitempty"` | ||
| InjectionConfig *InjectionConfig `json:"injectionConfig,omitempty"` | ||
| } | ||
|
|
||
| // ContainerConfig holds the environment variables and CA certificate returned | ||
| // by the OneCLI /api/container-config endpoint. | ||
| type ContainerConfig struct { | ||
| Env map[string]string `json:"env"` | ||
| CACertificate string `json:"caCertificate"` | ||
| CACertificateContainerPath string `json:"caCertificateContainerPath"` | ||
| } | ||
|
|
||
| // Secret represents a secret returned by the OneCLI API. | ||
| type Secret struct { | ||
| ID string `json:"id"` | ||
| Name string `json:"name"` | ||
| Type string `json:"type"` | ||
| HostPattern string `json:"hostPattern"` | ||
| PathPattern *string `json:"pathPattern"` | ||
| InjectionConfig *InjectionConfig `json:"injectionConfig"` | ||
| CreatedAt string `json:"createdAt"` | ||
| } | ||
|
|
||
| // InjectionConfig describes how a secret is injected into HTTP requests. | ||
| type InjectionConfig struct { | ||
| HeaderName string `json:"headerName"` | ||
| ValueFormat string `json:"valueFormat,omitempty"` | ||
| } | ||
|
|
||
| // CreateSecretInput is the request body for creating a secret. | ||
| type CreateSecretInput struct { | ||
| Name string `json:"name"` | ||
| Type string `json:"type"` | ||
| Value string `json:"value"` | ||
| HostPattern string `json:"hostPattern"` | ||
| PathPattern string `json:"pathPattern,omitempty"` | ||
| InjectionConfig *InjectionConfig `json:"injectionConfig,omitempty"` | ||
| } | ||
|
|
||
| // APIError represents an error response from the OneCLI API. | ||
| type APIError struct { | ||
| StatusCode int | ||
| Message string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid exporting concrete OneCLI models from pkg.
These newly exported structs (CreateSecretInput, InjectionConfig, Secret, ContainerConfig, UpdateSecretInput, APIError) violate the package API rule. Consider exposing interfaces/factories/accessors, moving DTOs to an allowed API package, or documenting an explicit exception if these JSON contracts must remain public. As per coding guidelines, pkg/**/*.go: All public types must be declared as interfaces; implementations must be unexported (lowercase names) with compile-time interface checks using unnamed variable declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/onecli/client.go` around lines 42 - 89, The structs Exported from pkg
(CreateSecretInput, UpdateSecretInput, InjectionConfig, Secret, ContainerConfig,
APIError) violate the pkg public-type rule; fix by making these DTOs unexported
(rename to createSecretInput, updateSecretInput, injectionConfig, secret,
containerConfig, apiError) and expose only exported interfaces or
factory/accessor functions in this package (e.g., NewCreateSecretInput,
CreateSecretInputDTO interface, or GetSecret accessor) or alternatively move the
concrete JSON DTOs into an allowed API package and keep only interfaces in pkg;
ensure any compile-time interface assertions remain (unnamed var _: Interface =
(*concrete)(nil)) and update usages of functions referencing CreateSecretInput,
UpdateSecretInput, InjectionConfig, Secret, ContainerConfig, and APIError
accordingly.
Contributor
Author
There was a problem hiding this comment.
these are DTOs (data transfer objects) for the OneCLI API. Wrapping them in interfaces adds complexity with no real benefit — they're just data bags, not behaviors.
@jeffmaury @feloy any opinion? I know fuckall about go
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if err != nil { | ||
| return fmt.Errorf("executing request: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Handle the response body close explicitly.
errcheck flags this deferred close. Explicitly discard the close error if it is intentionally non-actionable after io.ReadAll.
Proposed fix
- defer resp.Body.Close()
+ defer func() {
+ _ = resp.Body.Close()
+ }()📝 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
| defer resp.Body.Close() | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 184-184: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/onecli/client.go` at line 184, Replace the bare defer resp.Body.Close()
with an explicit discard of the Close error so errcheck is satisfied; after
calling io.ReadAll on resp.Body, use a deferred closure like defer func() { _ =
resp.Body.Close() }() (or check and assign cerr := resp.Body.Close(); _ = cerr)
to explicitly ignore the error from resp.Body.Close().
Comment on lines
+32
to
+37
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Path != "/api/user/api-key" { | ||
| t.Errorf("unexpected path: %s", r.URL.Path) | ||
| } | ||
| json.NewEncoder(w).Encode(map[string]string{"apiKey": "oc_test_key_123"}) | ||
| })) |
There was a problem hiding this comment.
Check response writes in test handlers.
These handlers currently ignore Encode/Write errors, which is flagged by errcheck.
Proposed fix
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api/user/api-key" {
t.Errorf("unexpected path: %s", r.URL.Path)
}
- json.NewEncoder(w).Encode(map[string]string{"apiKey": "oc_test_key_123"})
+ if err := json.NewEncoder(w).Encode(map[string]string{"apiKey": "oc_test_key_123"}); err != nil {
+ t.Errorf("failed to encode response: %v", err)
+ }
})) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
- w.Write([]byte("internal error"))
+ if _, err := w.Write([]byte("internal error")); err != nil {
+ t.Errorf("failed to write response: %v", err)
+ }
})) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
- json.NewEncoder(w).Encode(map[string]string{"apiKey": "bad_prefix"})
+ if err := json.NewEncoder(w).Encode(map[string]string{"apiKey": "bad_prefix"}); err != nil {
+ t.Errorf("failed to encode response: %v", err)
+ }
}))Also applies to: 53-56, 69-71
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 36-36: Error return value of (*encoding/json.Encoder).Encode is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/onecli/credentials_test.go` around lines 32 - 37, Test HTTP handlers in
credentials_test.go are ignoring errors from json.NewEncoder(w).Encode (and any
w.Write) which trips errcheck; update each httptest.NewServer handler closure
(the anonymous http.HandlerFunc where json.NewEncoder(w).Encode(...) is called
at the shown locations and the other occurrences around lines 53-56 and 69-71)
to capture the error return and fail the test on error (e.g., if err :=
json.NewEncoder(w).Encode(...); err != nil { t.Fatalf("encode response failed:
%v", err) }) or similarly handle write errors from w.Write, so all Encode/Write
calls return-checked and tests surface failures.
| if err != nil { | ||
| return "", fmt.Errorf("requesting API key: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Handle the response body close explicitly.
errcheck flags the deferred close here. If the close error is intentionally irrelevant after reading the body, discard it explicitly.
Proposed fix
- defer resp.Body.Close()
+ defer func() {
+ _ = resp.Body.Close()
+ }()📝 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
| defer resp.Body.Close() | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 68-68: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/onecli/credentials.go` at line 68, Replace the bare deferred call to
resp.Body.Close() with an explicit discard of its return value to satisfy
errcheck: change the defer resp.Body.Close() to a defer func() { _ =
resp.Body.Close() }() (optionally drain the body with io.Copy(io.Discard,
resp.Body) before closing if needed). Update the code in the function that uses
resp (the place where resp.Body.Close() is currently deferred) so the close
error is explicitly ignored by assigning it to the blank identifier.
Comment on lines
+583
to
+584
| caFile := filepath.Join(t.TempDir(), "ca.pem") | ||
| os.WriteFile(caFile, []byte("cert-data"), 0644) |
There was a problem hiding this comment.
Check the CA fixture write error.
If this write fails, the test continues with a missing CA file and reports a misleading buildContainerArgs failure.
Proposed fix
caFile := filepath.Join(t.TempDir(), "ca.pem")
- os.WriteFile(caFile, []byte("cert-data"), 0644)
+ if err := os.WriteFile(caFile, []byte("cert-data"), 0644); err != nil {
+ t.Fatalf("failed to write CA fixture: %v", err)
+ }📝 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
| caFile := filepath.Join(t.TempDir(), "ca.pem") | |
| os.WriteFile(caFile, []byte("cert-data"), 0644) | |
| caFile := filepath.Join(t.TempDir(), "ca.pem") | |
| if err := os.WriteFile(caFile, []byte("cert-data"), 0644); err != nil { | |
| t.Fatalf("failed to write CA fixture: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create_test.go` around lines 583 - 584, The test currently
writes the CA fixture with os.WriteFile(caFile, []byte("cert-data"), 0644) but
ignores its error; update the test to check the returned error (e.g., err :=
os.WriteFile(...); if err != nil { t.Fatalf("failed to write CA fixture: %v",
err) }) so the test fails immediately on write problems and doesn't proceed with
a missing CA file when buildContainerArgs runs.
Comment on lines
+654
to
+670
| // Find the indices of both -e HTTP_PROXY entries | ||
| var onecliIdx, wsIdx int | ||
| for i, arg := range args { | ||
| if arg == "-e" && i+1 < len(args) { | ||
| if args[i+1] == "HTTP_PROXY=http://onecli-proxy:8080" { | ||
| onecliIdx = i | ||
| } | ||
| if args[i+1] == "HTTP_PROXY=http://user-proxy:9090" { | ||
| wsIdx = i | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // OneCLI env var should come after workspace env var (later wins in podman) | ||
| if onecliIdx <= wsIdx { | ||
| t.Errorf("OneCLI HTTP_PROXY (index %d) should come after workspace HTTP_PROXY (index %d) for precedence", onecliIdx, wsIdx) | ||
| } |
There was a problem hiding this comment.
Use sentinel indices so missing env vars fail the test.
onecliIdx and wsIdx default to 0, so a missing workspace env var can still pass the ordering assertion when the OneCLI entry is found later.
Proposed fix
// Find the indices of both -e HTTP_PROXY entries
- var onecliIdx, wsIdx int
+ onecliIdx, wsIdx := -1, -1
for i, arg := range args {
if arg == "-e" && i+1 < len(args) {
if args[i+1] == "HTTP_PROXY=http://onecli-proxy:8080" {
onecliIdx = i
@@
}
}
}
+ if onecliIdx == -1 || wsIdx == -1 {
+ t.Fatalf("expected both OneCLI and workspace HTTP_PROXY entries, got args: %v", args)
+ }
+
// OneCLI env var should come after workspace env var (later wins in podman)
if onecliIdx <= wsIdx {📝 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
| // Find the indices of both -e HTTP_PROXY entries | |
| var onecliIdx, wsIdx int | |
| for i, arg := range args { | |
| if arg == "-e" && i+1 < len(args) { | |
| if args[i+1] == "HTTP_PROXY=http://onecli-proxy:8080" { | |
| onecliIdx = i | |
| } | |
| if args[i+1] == "HTTP_PROXY=http://user-proxy:9090" { | |
| wsIdx = i | |
| } | |
| } | |
| } | |
| // OneCLI env var should come after workspace env var (later wins in podman) | |
| if onecliIdx <= wsIdx { | |
| t.Errorf("OneCLI HTTP_PROXY (index %d) should come after workspace HTTP_PROXY (index %d) for precedence", onecliIdx, wsIdx) | |
| } | |
| // Find the indices of both -e HTTP_PROXY entries | |
| onecliIdx, wsIdx := -1, -1 | |
| for i, arg := range args { | |
| if arg == "-e" && i+1 < len(args) { | |
| if args[i+1] == "HTTP_PROXY=http://onecli-proxy:8080" { | |
| onecliIdx = i | |
| } | |
| if args[i+1] == "HTTP_PROXY=http://user-proxy:9090" { | |
| wsIdx = i | |
| } | |
| } | |
| } | |
| if onecliIdx == -1 || wsIdx == -1 { | |
| t.Fatalf("expected both OneCLI and workspace HTTP_PROXY entries, got args: %v", args) | |
| } | |
| // OneCLI env var should come after workspace env var (later wins in podman) | |
| if onecliIdx <= wsIdx { | |
| t.Errorf("OneCLI HTTP_PROXY (index %d) should come after workspace HTTP_PROXY (index %d) for precedence", onecliIdx, wsIdx) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create_test.go` around lines 654 - 670, Initialize the
sentinel indices onecliIdx and wsIdx to -1 (not 0), then after the loop that
scans args verify both were found (i.e. onecliIdx != -1 and wsIdx != -1) and
fail the test with a clear message if either is missing; only then assert
ordering (onecliIdx > wsIdx) to ensure a missing env var cannot incorrectly
satisfy the precedence check — update the variables and add the presence checks
around the existing loop in the test that inspects args.
Comment on lines
358
to
362
| stepLogger.Start(fmt.Sprintf("Creating workspace container: %s", params.Name), "Workspace container created") | ||
| createArgs, err := p.buildContainerArgs(params, imageName) | ||
| createArgs, err := p.buildContainerArgs(params, imageName, ccArgs) | ||
| if err != nil { | ||
| return runtime.RuntimeInfo{}, err | ||
| } |
There was a problem hiding this comment.
Mark the workspace-container step as failed when args cannot be built.
buildContainerArgs can fail, but this error path skips stepLogger.Fail(err) after the step has started.
Proposed fix
stepLogger.Start(fmt.Sprintf("Creating workspace container: %s", params.Name), "Workspace container created")
createArgs, err := p.buildContainerArgs(params, imageName, ccArgs)
if err != nil {
+ stepLogger.Fail(err)
return runtime.RuntimeInfo{}, err
}📝 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
| stepLogger.Start(fmt.Sprintf("Creating workspace container: %s", params.Name), "Workspace container created") | |
| createArgs, err := p.buildContainerArgs(params, imageName) | |
| createArgs, err := p.buildContainerArgs(params, imageName, ccArgs) | |
| if err != nil { | |
| return runtime.RuntimeInfo{}, err | |
| } | |
| stepLogger.Start(fmt.Sprintf("Creating workspace container: %s", params.Name), "Workspace container created") | |
| createArgs, err := p.buildContainerArgs(params, imageName, ccArgs) | |
| if err != nil { | |
| stepLogger.Fail(err) | |
| return runtime.RuntimeInfo{}, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create.go` around lines 358 - 362, When buildContainerArgs
fails after calling stepLogger.Start, mark the step as failed before returning:
in the function where stepLogger.Start("Creating workspace container: %s",
params.Name) is called and buildContainerArgs(params, imageName, ccArgs) returns
an error, call stepLogger.Fail(err) (including the error) and then return the
runtime.RuntimeInfo and error; update the error path that currently just returns
to ensure stepLogger.Fail(err) is invoked so the workspace-container step is
recorded as failed.
Comment on lines
+88
to
+90
| // cleanupCerts removes the CA certificate directory for a workspace. | ||
| func (p *podmanRuntime) cleanupCerts(podName string) { | ||
| os.RemoveAll(filepath.Join(p.storageDir, "certs", podName)) |
There was a problem hiding this comment.
Guard RemoveAll against path traversal.
podName is joined directly into a deletion path. A value containing .. can escape storageDir/certs after filepath.Clean, making RemoveAll destructive outside the intended workspace cert directory.
Proposed fix
// cleanupCerts removes the CA certificate directory for a workspace.
-func (p *podmanRuntime) cleanupCerts(podName string) {
- os.RemoveAll(filepath.Join(p.storageDir, "certs", podName))
+func (p *podmanRuntime) cleanupCerts(podName string) error {
+ certRoot := filepath.Join(p.storageDir, "certs")
+ target := filepath.Clean(filepath.Join(certRoot, podName))
+ rel, err := filepath.Rel(certRoot, target)
+ if err != nil || rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+ return fmt.Errorf("refusing to remove cert directory outside cert root: %q", podName)
+ }
+ return os.RemoveAll(target)
}Update callers to handle the returned error:
- p.cleanupCerts(podName)
+ if err := p.cleanupCerts(podName); err != nil {
+ stepLogger.Fail(err)
+ return err
+ }📝 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
| // cleanupCerts removes the CA certificate directory for a workspace. | |
| func (p *podmanRuntime) cleanupCerts(podName string) { | |
| os.RemoveAll(filepath.Join(p.storageDir, "certs", podName)) | |
| // cleanupCerts removes the CA certificate directory for a workspace. | |
| func (p *podmanRuntime) cleanupCerts(podName string) error { | |
| certRoot := filepath.Join(p.storageDir, "certs") | |
| target := filepath.Clean(filepath.Join(certRoot, podName)) | |
| rel, err := filepath.Rel(certRoot, target) | |
| if err != nil || rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { | |
| return fmt.Errorf("refusing to remove cert directory outside cert root: %q", podName) | |
| } | |
| return os.RemoveAll(target) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/remove.go` around lines 88 - 90, The cleanupCerts function
currently calls os.RemoveAll(filepath.Join(p.storageDir, "certs", podName))
without sanitizing podName or returning errors; update cleanupCerts on
podmanRuntime to first clean and validate the joined path (use filepath.Clean
and ensure the resulting path has p.storageDir/certs as its prefix to prevent
path traversal), call os.RemoveAll on the validated path, and return the error
instead of ignoring it so callers can handle failure; reference cleanupCerts,
podName, p.storageDir, and os.RemoveAll in your changes.
Comment on lines
+99
to
+108
| func (p *podmanRuntime) installCACert(ctx context.Context, containerID, caContainerPath string) error { | ||
| l := logger.FromContext(ctx) | ||
| cmd := fmt.Sprintf("sudo cp %s /etc/pki/ca-trust/source/anchors/onecli-ca.pem && sudo update-ca-trust", caContainerPath) | ||
| if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(), | ||
| "exec", containerID, "sh", "-c", cmd, | ||
| ); err != nil { | ||
| return fmt.Errorf("update-ca-trust failed: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Avoid shell interpolation when installing the CA cert.
caContainerPath is persisted from the OneCLI response and is interpolated into sh -c unquoted. Run cp and update-ca-trust as separate exec calls, and build the container trust path with path.Join().
Proposed fix
import (
"context"
"fmt"
"io"
"net/http"
+ "path"
"time" func (p *podmanRuntime) installCACert(ctx context.Context, containerID, caContainerPath string) error {
l := logger.FromContext(ctx)
- cmd := fmt.Sprintf("sudo cp %s /etc/pki/ca-trust/source/anchors/onecli-ca.pem && sudo update-ca-trust", caContainerPath)
+
+ trustAnchorPath := path.Join("/etc", "pki", "ca-trust", "source", "anchors", "onecli-ca.pem")
if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(),
- "exec", containerID, "sh", "-c", cmd,
+ "exec", containerID, "sudo", "cp", caContainerPath, trustAnchorPath,
+ ); err != nil {
+ return fmt.Errorf("copy CA certificate failed: %w", err)
+ }
+ if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(),
+ "exec", containerID, "sudo", "update-ca-trust",
); err != nil {
return fmt.Errorf("update-ca-trust failed: %w", err)
}As per coding guidelines, “All path operations must use filepath.Join() for host paths (files on disk) and path.Join() for container paths (inside Podman).”
📝 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
| func (p *podmanRuntime) installCACert(ctx context.Context, containerID, caContainerPath string) error { | |
| l := logger.FromContext(ctx) | |
| cmd := fmt.Sprintf("sudo cp %s /etc/pki/ca-trust/source/anchors/onecli-ca.pem && sudo update-ca-trust", caContainerPath) | |
| if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(), | |
| "exec", containerID, "sh", "-c", cmd, | |
| ); err != nil { | |
| return fmt.Errorf("update-ca-trust failed: %w", err) | |
| } | |
| return nil | |
| } | |
| func (p *podmanRuntime) installCACert(ctx context.Context, containerID, caContainerPath string) error { | |
| l := logger.FromContext(ctx) | |
| trustAnchorPath := path.Join("/etc", "pki", "ca-trust", "source", "anchors", "onecli-ca.pem") | |
| if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(), | |
| "exec", containerID, "sudo", "cp", caContainerPath, trustAnchorPath, | |
| ); err != nil { | |
| return fmt.Errorf("copy CA certificate failed: %w", err) | |
| } | |
| if err := p.executor.Run(ctx, l.Stdout(), l.Stderr(), | |
| "exec", containerID, "sudo", "update-ca-trust", | |
| ); err != nil { | |
| return fmt.Errorf("update-ca-trust failed: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/start.go` around lines 99 - 108, installCACert currently
interpolates caContainerPath into a sh -c string which risks shell injection;
change it to build the destination path using path.Join for the container trust
dir and invoke two separate p.executor.Run exec calls (one for "cp" with args:
containerID, caContainerPath, joined destination path; and one for
"update-ca-trust" with args: containerID, "update-ca-trust") instead of a single
"sh -c" call so no shell interpolation is used and each command is executed
directly.
Comment on lines
+149
to
+154
| resp, err := httpClient.Do(req) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| if resp.StatusCode < 500 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Require a successful health response.
StatusCode < 500 treats 404 and 401 as ready, which can mask a wrong base URL or missing health route.
Proposed fix
resp, err := httpClient.Do(req)
if err == nil {
resp.Body.Close()
- if resp.StatusCode < 500 {
+ if resp.StatusCode >= 200 && resp.StatusCode < 300 {
return nil
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/start.go` around lines 149 - 154, The current readiness
check treats any status <500 as success (e.g., 401/404); update the check after
httpClient.Do(req) so it only returns success for a genuine successful health
response (e.g., require resp.StatusCode >= 200 && resp.StatusCode < 300 or
another explicit success range you prefer) instead of resp.StatusCode < 500;
keep the resp.Body.Close() behavior and ensure the function continues retrying
for non-success codes like 401/404.
Postgres and the OneCLI gateway proxy no longer need host-exposed ports since all communication happens within the pod's shared network namespace. Only the OneCLI web port is exposed for secret provisioning during setup. Also changes restartPolicy to Never since we manage container start order ourselves (postgres before onecli) to avoid migration races. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
fbricon
force-pushed
the
worktree-GH-266
branch
from
3c4e09e to
153eb79
Compare
…iden#266) Add OneCLI secret provisioning during workspace creation. Secrets defined in workspace configuration are mapped to OneCLI API format and provisioned via the credential gateway. On 409 conflict, existing secrets are updated with new values. Includes: OneCLI API client, secret mapper, provisioner, credential provider, CA certificate handling, certs cleanup on workspace removal, and context.Background() for defer cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
OneCLI expects literal hostnames, not regex patterns. The escaped value `api\.github\.com` was being sent as-is, causing host matching to fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
On container start, copy the OneCLI CA certificate into /etc/pki/ca-trust/source/anchors/ and run update-ca-trust so all tools (gh, curl, Go binaries) trust the OneCLI proxy. The CA container path is persisted in pod metadata during create. Adds /usr/bin/cp and /usr/bin/update-ca-trust to the default sudo allowlist (both already present in base Fedora image). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
fbricon
force-pushed
the
worktree-GH-266
branch
from
153eb79 to
5ff3fce
Compare
Contributor
|
On Mac I can see that the pod has started correctly but I get this error with CA |
Contributor
Author
right I got this too initially, you need to delete ~/.kdn/runtimes/podman/config/image.json or manually add these to the sudo list: then delete the workspace image and try again I figured this is something kdn will have to handle better, detecting changes to its cached config files, but that's out of scope of the PR IMHO |
jeffmaury
approved these changes
Apr 21, 2026
Contributor
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
LGTM
Tested on Windows
gastoner
approved these changes
Apr 22, 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
4 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.
Map workspace secrets to OneCLI secret definitions and provision them
during workspace creation. The OneCLI proxy env vars and CA certificate
are injected into the workspace container for transparent credential
injection via the gateway.
The way I tested it:
./kdn init /path/to/my-project --runtime podman --agent opencode --show-logs./kdn start my-project./kdn terminal my-projectopencode should open/modelsthenctrl+ato select the providerOllama Cloudand enter a fake value for the API Key (FAKE_API_KEY)kimi-k2.5)! env | grep TOKEN, it should show the GH_TOKEN and GITHUB_TOKEN have aplaceholdervalue! gh auth status, it should should you're correctly authenticated with GithubThe onecli web port is exposed to the host, so you can access it and change the Ollama key value, to something wrong. When you return to the opencode tui, next interaction will show
Unauthorized: unauthorizedFixes #266 and #295