fix(agent): update GetClientWithRetry to use fabricAddr and enhance e…#2104
fix(agent): update GetClientWithRetry to use fabricAddr and enhance e…#2104
Conversation
…rror messages for stack context Agent must call Authenticate to auth to the cloud
📝 WalkthroughWalkthroughThis PR refactors agent tools to accept fabric addresses directly instead of config objects, introduces provider authentication, expands tool messages with stack context, and adds a new project name tool. ChangesAgent Tool Fabric Client Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/agent/tools/auth.go`:
- Around line 10-22: GetClientWithRetry currently returns raw errors from
cli.Connect and cli.InteractiveLoginMCP; update it to wrap each returned error
with contextual messages using fmt.Errorf(... %w) that identify the failing step
(e.g., "connect to fabricAddr", "interactive login to MCP") and include a short
remediation hint for the user (e.g., "check credentials or run `auth login`").
Specifically wrap errors returned by the first cli.Connect call, the
InteractiveLoginMCP call, and the subsequent reconnect cli.Connect call,
referencing the function GetClientWithRetry and the CLIInterface methods Connect
and InteractiveLoginMCP so callers receive step-level context and actionable
guidance.
In `@src/pkg/agent/tools/project_name.go`:
- Around line 15-19: HandleProjectNameTool returns the raw error from
loader.LoadProjectName which loses context and remediation; update the error
return in HandleProjectNameTool to wrap the underlying err using fmt.Errorf with
%w and add a user-facing hint (e.g., "please retry the operation" or steps to
retry) so callers receive contextual and actionable guidance when
loader.LoadProjectName fails.
In `@src/pkg/agent/tools/provider.go`:
- Around line 53-55: The error returned from provider.Authenticate currently
omits remediation steps; update the error returned from the Authenticate call
(the block using provider.Authenticate(ctx, pp.ec.IsSupported()) and referencing
stack.Provider) to append a user-facing remediation suggestion such as asking
the user to re-run authentication or log in again (e.g., "please retry
authentication or run `provider login`/re-authenticate via the UI"), ensuring
the formatted error still wraps the original err (use fmt.Errorf with %w) and
includes the provider name and the explicit retry/login instruction.
In `@src/pkg/agent/tools/removeConfig_test.go`:
- Around line 39-41: The MockRemoveConfigCLI.NewProvider currently logs only
providerId, missing stack (and fabric) so stack plumbing regressions won't be
caught; update MockRemoveConfigCLI.NewProvider to include the stack (and
optionally fabric) when appending to CallLog (e.g., "NewProvider(providerId,
stack=...)" or include fabric identifier) and then update the test's
expectedCalls in removeConfig_test.go to assert the stack value is present;
locate the NewProvider method and the expectedCalls table entries to keep test
table-driven and ensure the stack is asserted.
In `@src/pkg/agent/tools/removeConfig.go`:
- Line 54: The error returned when removing a config variable (the fmt.Errorf
line referencing params.Name, projectName, and sc.Stack.Name) lacks remediation
guidance; update that return to append a brief remediation hint advising the
user to verify the project/stack names and permissions and retry (while
preserving the original wrapped error via %w). Locate the failing return in the
remove config flow (the statement that returns fmt.Errorf("failed to remove
config variable %q from project %q in stack %q: %w", params.Name, projectName,
sc.Stack.Name, err)) and extend the message to include a short action like
"Please verify project/stack names and your permissions, then retry." while
keeping error wrapping intact.
In `@src/pkg/agent/tools/tools.go`:
- Around line 139-142: The return here uses a generic message and returns the
raw err; wrap the underlying error with fmt.Errorf using %w to preserve the
cause (e.g., fmt.Errorf("failed to configure agent loader: %w", err)) and change
the user-facing string to include a remediation hint such as "Failed to
configure loader — check loader params and retry" so callers see actionable
guidance; apply the same pattern for other occurrences in this file where
common.ConfigureAgentLoader or similar loader/config functions return errors
(ensure you reference ConfigureAgentLoader, loader, and the surrounding function
when making edits).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20ecbebf-9048-4ed3-a8f8-8554a70f4067
📒 Files selected for processing (18)
src/pkg/agent/tools/auth.gosrc/pkg/agent/tools/deploy.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/destroy.gosrc/pkg/agent/tools/destroy_test.gosrc/pkg/agent/tools/estimate.gosrc/pkg/agent/tools/listConfig.gosrc/pkg/agent/tools/listConfig_test.gosrc/pkg/agent/tools/logs.gosrc/pkg/agent/tools/project_name.gosrc/pkg/agent/tools/provider.gosrc/pkg/agent/tools/removeConfig.gosrc/pkg/agent/tools/removeConfig_test.gosrc/pkg/agent/tools/services.gosrc/pkg/agent/tools/setConfig.gosrc/pkg/agent/tools/setConfig_test.gosrc/pkg/agent/tools/tools.gosrc/pkg/cli/client/mock.go
| func GetClientWithRetry(ctx context.Context, cli CLIInterface, fabricAddr string) (*client.GrpcClient, error) { | ||
| client, err := cli.Connect(ctx, fabricAddr) | ||
| if err != nil { | ||
| err = cli.InteractiveLoginMCP(ctx, config.FabricAddr, common.MCPDevelopmentClient) | ||
| err = cli.InteractiveLoginMCP(ctx, fabricAddr, common.MCPDevelopmentClient) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Reconnect with the new token | ||
| client, err = cli.Connect(ctx, config.FabricAddr) | ||
| client, err = cli.Connect(ctx, fabricAddr) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add contextual wrapping for auth/reconnect failures in retry flow.
Both login and reconnect failures are returned raw, so callers lose step-level context and actionable guidance.
💡 Proposed fix
import (
"context"
+ "fmt"
"github.com/DefangLabs/defang/src/pkg/agent/common"
"github.com/DefangLabs/defang/src/pkg/cli/client"
)
@@
func GetClientWithRetry(ctx context.Context, cli CLIInterface, fabricAddr string) (*client.GrpcClient, error) {
client, err := cli.Connect(ctx, fabricAddr)
if err != nil {
err = cli.InteractiveLoginMCP(ctx, fabricAddr, common.MCPDevelopmentClient)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to authenticate to cloud; please authenticate and retry: %w", err)
}
@@
client, err = cli.Connect(ctx, fabricAddr)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to reconnect to fabric after authentication: %w", err)
}
}As per coding guidelines, **/*.go: “Wrap errors with context using fmt.Errorf with %w verb” and “User-facing errors must include remediation steps”.
📝 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.
| func GetClientWithRetry(ctx context.Context, cli CLIInterface, fabricAddr string) (*client.GrpcClient, error) { | |
| client, err := cli.Connect(ctx, fabricAddr) | |
| if err != nil { | |
| err = cli.InteractiveLoginMCP(ctx, config.FabricAddr, common.MCPDevelopmentClient) | |
| err = cli.InteractiveLoginMCP(ctx, fabricAddr, common.MCPDevelopmentClient) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Reconnect with the new token | |
| client, err = cli.Connect(ctx, config.FabricAddr) | |
| client, err = cli.Connect(ctx, fabricAddr) | |
| if err != nil { | |
| return nil, err | |
| } | |
| import ( | |
| "context" | |
| "fmt" | |
| "github.com/DefangLabs/defang/src/pkg/agent/common" | |
| "github.com/DefangLabs/defang/src/pkg/cli/client" | |
| ) | |
| func GetClientWithRetry(ctx context.Context, cli CLIInterface, fabricAddr string) (*client.GrpcClient, error) { | |
| client, err := cli.Connect(ctx, fabricAddr) | |
| if err != nil { | |
| err = cli.InteractiveLoginMCP(ctx, fabricAddr, common.MCPDevelopmentClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to authenticate to cloud; please authenticate and retry: %w", err) | |
| } | |
| // Reconnect with the new token | |
| client, err = cli.Connect(ctx, fabricAddr) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to reconnect to fabric after authentication: %w", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/agent/tools/auth.go` around lines 10 - 22, GetClientWithRetry
currently returns raw errors from cli.Connect and cli.InteractiveLoginMCP;
update it to wrap each returned error with contextual messages using
fmt.Errorf(... %w) that identify the failing step (e.g., "connect to
fabricAddr", "interactive login to MCP") and include a short remediation hint
for the user (e.g., "check credentials or run `auth login`"). Specifically wrap
errors returned by the first cli.Connect call, the InteractiveLoginMCP call, and
the subsequent reconnect cli.Connect call, referencing the function
GetClientWithRetry and the CLIInterface methods Connect and InteractiveLoginMCP
so callers receive step-level context and actionable guidance.
| func HandleProjectNameTool(ctx context.Context, loader client.Loader) (string, error) { | ||
| pn, _, err := loader.LoadProjectName(ctx) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Wrap project-name load failures with actionable context.
Returning the raw error here drops operation context and remediation guidance; wrap it with %w and include a retry hint.
💡 Proposed fix
import (
"context"
+ "fmt"
"github.com/DefangLabs/defang/src/pkg/agent/common"
"github.com/DefangLabs/defang/src/pkg/cli/client"
)
@@
func HandleProjectNameTool(ctx context.Context, loader client.Loader) (string, error) {
pn, _, err := loader.LoadProjectName(ctx)
if err != nil {
- return "", err
+ return "", fmt.Errorf("failed to load project name; please verify project context and retry: %w", err)
}
return pn, nil
}As per coding guidelines, **/*.go: “Wrap errors with context using fmt.Errorf with %w verb” and “User-facing errors must include remediation steps”.
📝 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.
| func HandleProjectNameTool(ctx context.Context, loader client.Loader) (string, error) { | |
| pn, _, err := loader.LoadProjectName(ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| import ( | |
| "context" | |
| "fmt" | |
| "github.com/DefangLabs/defang/src/pkg/agent/common" | |
| "github.com/DefangLabs/defang/src/pkg/cli/client" | |
| ) | |
| func HandleProjectNameTool(ctx context.Context, loader client.Loader) (string, error) { | |
| pn, _, err := loader.LoadProjectName(ctx) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to load project name; please verify project context and retry: %w", err) | |
| } | |
| return pn, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/agent/tools/project_name.go` around lines 15 - 19,
HandleProjectNameTool returns the raw error from loader.LoadProjectName which
loses context and remediation; update the error return in HandleProjectNameTool
to wrap the underlying err using fmt.Errorf with %w and add a user-facing hint
(e.g., "please retry the operation" or steps to retry) so callers receive
contextual and actionable guidance when loader.LoadProjectName fails.
| if err := provider.Authenticate(ctx, pp.ec.IsSupported()); err != nil { | ||
| return nil, nil, fmt.Errorf("failed to authenticate with provider %q: %w", stack.Provider, err) | ||
| } |
There was a problem hiding this comment.
Add remediation guidance to authentication failure message.
Line 54 returns an actionable, user-facing auth error but doesn’t tell the user what to do next. Please include a retry/login step in the message.
Suggested patch
- if err := provider.Authenticate(ctx, pp.ec.IsSupported()); err != nil {
- return nil, nil, fmt.Errorf("failed to authenticate with provider %q: %w", stack.Provider, err)
- }
+ if err := provider.Authenticate(ctx, pp.ec.IsSupported()); err != nil {
+ return nil, nil, fmt.Errorf(
+ "failed to authenticate with provider %q: %w. Please sign in again and retry.",
+ stack.Provider,
+ err,
+ )
+ }As per coding guidelines, "User-facing errors must include remediation steps".
📝 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.
| if err := provider.Authenticate(ctx, pp.ec.IsSupported()); err != nil { | |
| return nil, nil, fmt.Errorf("failed to authenticate with provider %q: %w", stack.Provider, err) | |
| } | |
| if err := provider.Authenticate(ctx, pp.ec.IsSupported()); err != nil { | |
| return nil, nil, fmt.Errorf( | |
| "failed to authenticate with provider %q: %w. Please sign in again and retry.", | |
| stack.Provider, | |
| err, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/agent/tools/provider.go` around lines 53 - 55, The error returned
from provider.Authenticate currently omits remediation steps; update the error
returned from the Authenticate call (the block using provider.Authenticate(ctx,
pp.ec.IsSupported()) and referencing stack.Provider) to append a user-facing
remediation suggestion such as asking the user to re-run authentication or log
in again (e.g., "please retry authentication or run `provider
login`/re-authenticate via the UI"), ensuring the formatted error still wraps
the original err (use fmt.Errorf with %w) and includes the provider name and the
explicit retry/login instruction.
| func (m *MockRemoveConfigCLI) NewProvider(ctx context.Context, providerId client.ProviderID, fabric client.FabricClient, stack string) client.Provider { | ||
| m.CallLog = append(m.CallLog, fmt.Sprintf("NewProvider(%s)", providerId)) | ||
| return nil // Mock provider | ||
| return client.MockProvider{} |
There was a problem hiding this comment.
Assert stack propagation in NewProvider call log
The mock now accepts stack/fabric but doesn’t record either, so a stack-plumbing regression can pass silently. Capture at least stack in CallLog and assert it in expectedCalls.
Proposed fix
func (m *MockRemoveConfigCLI) NewProvider(ctx context.Context, providerId client.ProviderID, fabric client.FabricClient, stack string) client.Provider {
- m.CallLog = append(m.CallLog, fmt.Sprintf("NewProvider(%s)", providerId))
+ m.CallLog = append(m.CallLog, fmt.Sprintf("NewProvider(%s, %s)", providerId, stack))
return client.MockProvider{}
}
@@
expectedCalls := []string{
"Connect(test-cluster)",
- "NewProvider(aws)",
+ "NewProvider(aws, test-stack)",
"LoadProjectNameWithFallback",
"ConfigDelete(test-project, DATABASE_URL)",
}As per coding guidelines: "Use table-driven tests for comprehensive test coverage in unit tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/agent/tools/removeConfig_test.go` around lines 39 - 41, The
MockRemoveConfigCLI.NewProvider currently logs only providerId, missing stack
(and fabric) so stack plumbing regressions won't be caught; update
MockRemoveConfigCLI.NewProvider to include the stack (and optionally fabric)
when appending to CallLog (e.g., "NewProvider(providerId, stack=...)" or include
fabric identifier) and then update the test's expectedCalls in
removeConfig_test.go to assert the stack value is present; locate the
NewProvider method and the expectedCalls table entries to keep test table-driven
and ensure the stack is asserted.
| return fmt.Sprintf("Config variable %q not found in project %q in stack %q", params.Name, projectName, sc.Stack.Name), nil | ||
| } | ||
| return "", fmt.Errorf("failed to remove config variable %q from project %q: %w", params.Name, projectName, err) | ||
| return "", fmt.Errorf("failed to remove config variable %q from project %q in stack %q: %w", params.Name, projectName, sc.Stack.Name, err) |
There was a problem hiding this comment.
Include remediation steps in the removal failure error.
Line 54 returns a user-facing failure but doesn’t provide next actions. Add a brief remediation hint (verify project/stack/permissions, then retry).
Suggested patch
- return "", fmt.Errorf("failed to remove config variable %q from project %q in stack %q: %w", params.Name, projectName, sc.Stack.Name, err)
+ return "", fmt.Errorf(
+ "failed to remove config variable %q from project %q in stack %q: %w. Please verify the project/stack and your permissions, then retry.",
+ params.Name,
+ projectName,
+ sc.Stack.Name,
+ err,
+ )As per coding guidelines, "User-facing errors must include remediation steps".
📝 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.
| return "", fmt.Errorf("failed to remove config variable %q from project %q in stack %q: %w", params.Name, projectName, sc.Stack.Name, err) | |
| return "", fmt.Errorf( | |
| "failed to remove config variable %q from project %q in stack %q: %w. Please verify the project/stack and your permissions, then retry.", | |
| params.Name, | |
| projectName, | |
| sc.Stack.Name, | |
| err, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/agent/tools/removeConfig.go` at line 54, The error returned when
removing a config variable (the fmt.Errorf line referencing params.Name,
projectName, and sc.Stack.Name) lacks remediation guidance; update that return
to append a brief remediation hint advising the user to verify the project/stack
names and permissions and retry (while preserving the original wrapped error via
%w). Locate the failing return in the remove config flow (the statement that
returns fmt.Errorf("failed to remove config variable %q from project %q in stack
%q: %w", params.Name, projectName, sc.Stack.Name, err)) and extend the message
to include a short action like "Please verify project/stack names and your
permissions, then retry." while keeping error wrapping intact.
| loader, err := common.ConfigureAgentLoader(params.LoaderParams) | ||
| if err != nil { | ||
| return "Failed to configure loader", err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify loader error branches and wrapping usage in this file
rg -n 'Failed to configure loader' src/pkg/agent/tools/tools.go
rg -n 'fmt\.Errorf\([^)]*%w' src/pkg/agent/tools/tools.goRepository: DefangLabs/defang
Length of output: 505
🏁 Script executed:
head -20 src/pkg/agent/tools/tools.goRepository: DefangLabs/defang
Length of output: 778
🏁 Script executed:
sed -n '130,150p' src/pkg/agent/tools/tools.goRepository: DefangLabs/defang
Length of output: 566
Wrap loader errors with context and add remediation in user-facing text
This branch returns a generic message and unwrapped error. Wrap the error with %w using fmt.Errorf and include a remediation hint in the user-facing text.
Proposed fix
import (
+ "fmt"
"github.com/DefangLabs/defang/src/pkg/agent/common"
"github.com/DefangLabs/defang/src/pkg/elicitations"
"github.com/firebase/genkit/go/ai"
)
@@
func(ctx *ai.ToolContext, params ProjectNameParams) (string, error) {
loader, err := common.ConfigureAgentLoader(params.LoaderParams)
if err != nil {
- return "Failed to configure loader", err
+ return "Failed to configure loader. Check your stack/project settings and try again.",
+ fmt.Errorf("configure agent loader for project_name tool: %w", err)
}
return HandleProjectNameTool(ctx, loader)
},
),Per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb" and "User-facing errors must include remediation steps". Note: This pattern appears in 9 locations across the file; consider addressing all instances.
📝 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.
| loader, err := common.ConfigureAgentLoader(params.LoaderParams) | |
| if err != nil { | |
| return "Failed to configure loader", err | |
| } | |
| loader, err := common.ConfigureAgentLoader(params.LoaderParams) | |
| if err != nil { | |
| return "Failed to configure loader. Check your stack/project settings and try again.", | |
| fmt.Errorf("configure agent loader for project_name tool: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/agent/tools/tools.go` around lines 139 - 142, The return here uses a
generic message and returns the raw err; wrap the underlying error with
fmt.Errorf using %w to preserve the cause (e.g., fmt.Errorf("failed to configure
agent loader: %w", err)) and change the user-facing string to include a
remediation hint such as "Failed to configure loader — check loader params and
retry" so callers see actionable guidance; apply the same pattern for other
occurrences in this file where common.ConfigureAgentLoader or similar
loader/config functions return errors (ensure you reference
ConfigureAgentLoader, loader, and the surrounding function when making edits).
…rror messages for stack context
Agent must call Authenticate to auth to the cloud
Description
Linked Issues
Checklist
Summary by CodeRabbit
New Features
project_nametool to retrieve the current project name.Improvements