fix(cli): correct mcp secrets sync apply behavior and thread cobra ctx#1814
Open
SarthakB11 wants to merge 3 commits intokagent-dev:mainfrom
Open
fix(cli): correct mcp secrets sync apply behavior and thread cobra ctx#1814SarthakB11 wants to merge 3 commits intokagent-dev:mainfrom
SarthakB11 wants to merge 3 commits intokagent-dev:mainfrom
Conversation
SyncSecretsMcp and applySecretToCluster passed context.TODO() to every clientset Get/Create/Update call against the kubernetes API. that means ctrl-c during a slow apply (e.g. against a remote cluster with a hung api-server connection) silently fell through to whatever the kubernetes client default did rather than honoring cancellation. thread cmd.Context() from the cobra RunE through both functions so the calls respect signal cancellation (and any future timeout the caller attaches). matches the existing kagent cli convention used by InstallCmd, UninstallCmd, InvokeCmd, DashboardCmd, and DeployCmd, which all take ctx as the first argument and are wired from cmd.Context() at the top level in cmd/kagent/main.go. Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in>
SarthakB11
requested review from
iplay88keys,
jmhbh and
supreme-gg-gg
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR threads the Cobra cmd.Context() through the MCP secrets sync path so Kubernetes API calls honor cancellation (e.g., Ctrl-C) instead of using context.TODO().
Changes:
- Updated
SyncSecretsMcpandapplySecretToClusterto accept acontext.Contextand pass it into Kubernetes clientGet/Create/Updatecalls. - Wired
cmd.Context()from thekagent mcp secrets syncCobra command intoSyncSecretsMcp.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| go/core/cli/internal/cli/mcp/secrets.go | Accepts/passes context.Context for Kubernetes secret apply operations. |
| go/core/cli/internal/cli/mcp/root.go | Passes cmd.Context() into SyncSecretsMcp for the secrets sync command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
128
to
134
| // Check if secret exists | ||
| _, err = clientset.CoreV1().Secrets(secret.Namespace).Get(context.TODO(), secret.Name, metav1.GetOptions{}) | ||
| _, err = clientset.CoreV1().Secrets(secret.Namespace).Get(ctx, secret.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| // Create if it doesn't exist | ||
| _, err = clientset.CoreV1().Secrets(secret.Namespace).Create(context.TODO(), secret, metav1.CreateOptions{}) | ||
| _, err = clientset.CoreV1().Secrets(secret.Namespace).Create(ctx, secret, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create secret: %w", err) |
Contributor
Author
There was a problem hiding this comment.
good catch. pushed cf8b5efd which switches on apierrors.IsNotFound(err): only true-NotFound triggers Create now, any other Get error (RBAC forbidden, network, ctx cancellation) returns a wrapped message immediately instead of falling through.
Comment on lines
137
to
141
| } else { | ||
| // Update if it exists | ||
| _, err = clientset.CoreV1().Secrets(secret.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{}) | ||
| _, err = clientset.CoreV1().Secrets(secret.Namespace).Update(ctx, secret, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update secret: %w", err) |
Contributor
Author
There was a problem hiding this comment.
good catch. pushed cf8b5efd which captures the live object from Get and copies existing.ResourceVersion onto the secret before calling Update. matches the kubernetes optimistic-concurrency contract.
…ion to Update per copilot review on PR kagent-dev#1814: (1) the previous code treated any Get error as 'secret does not exist' and proceeded to Create, so RBAC forbidden, network errors, and ctx-cancellation all surfaced as confusing 'failed to create secret' messages instead of the real cause. switch on apierrors.IsNotFound so only true-NotFound triggers Create; any other Get error returns a wrapped message immediately. (2) the update path discarded the Get result, so the Secret being sent to Update had no metadata.resourceVersion. kubernetes Update requires resourceVersion and rejects optimistic-concurrency-violating writes, so updates against a populated cluster would fail. capture the live object from Get and copy its resourceVersion onto the secret built from .env before calling Update. Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in>
SarthakB11
changed the title
github-actions
Bot
added
bug
EItanya
approved these changes
May 8, 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
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
three issues in
mcp secrets sync:SyncSecretsMcpandapplySecretToClusterpassedcontext.TODO()to every clientset Get/Create/Update call. ctrl-c during a slow apply (remote cluster, hung api-server connection) silently fell through to whatever the kubernetes client default did rather than honoring cancellation. fix: threadcmd.Context()from the cobraRunEthroughSyncSecretsMcpandapplySecretToCluster. matches the existing kagent cli convention used byInstallCmd,UninstallCmd,InvokeCmd,DashboardCmd, andDeployCmd, which all takectxas the first argument and are wired fromcmd.Context()at the top level incmd/kagent/main.go."failed to create secret"messages instead of the real cause. fix: branch onapierrors.IsNotFound(err)so only true-NotFound triggers Create; any other Get error returns a wrapped message immediately.metadata.resourceVersion. kubernetes Update requiresresourceVersionand rejects optimistic-concurrency-violating writes, so updates against a populated cluster would fail with a validation error. fix: capture the live object from Get and copy itsresourceVersiononto the secret built from.envbefore calling Update.the second and third items were flagged by copilot on the initial revision of this PR; addressed in the follow-up commit
cf8b5efd.ai model disclosure
used claude code (claude opus 4.7) to triage the
context.TODO()call sites and draft the diff. self-reviewed againstgo/core/cli/internal/cli/agent/deploy.goto confirm theapierrorsimport alias and the ctx-as-first-arg pattern, and againstcmd/kagent/main.goto confirmcmd.Context()is the established wiring point. verified locally via: