Skip to content

fix(agent): update GetClientWithRetry to use fabricAddr and enhance e…#2104

Merged
lionello merged 1 commit intomainfrom
lio/fix-agent
May 8, 2026
Merged

fix(agent): update GetClientWithRetry to use fabricAddr and enhance e…#2104
lionello merged 1 commit intomainfrom
lio/fix-agent

Conversation

@lionello
Copy link
Copy Markdown
Member

@lionello lionello commented May 8, 2026

…rror messages for stack context

Agent must call Authenticate to auth to the cloud

Description

Linked Issues

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • New Features

    • Added a project_name tool to retrieve the current project name.
    • Tools now properly authenticate with providers upon connection.
  • Improvements

    • Configuration operation messages now include stack context for improved clarity.
    • Tool descriptions clarified to specify they operate on the currently selected stack rather than the entire project.

…rror messages for stack context

Agent must call Authenticate to auth to the cloud
@lionello lionello requested a review from edwardrf May 8, 2026 01:20
@lionello lionello requested a review from jordanstephens as a code owner May 8, 2026 01:20
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This 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.

Changes

Agent Tool Fabric Client Refactoring

Layer / File(s) Summary
GetClientWithRetry Function Signature
src/pkg/agent/tools/auth.go
GetClientWithRetry parameter changes from StackConfig to fabricAddr string, used for initial connect, login MCP, and reconnect.
Provider Authentication
src/pkg/agent/tools/provider.go
SetupProvider now calls provider.Authenticate(ctx, pp.ec.IsSupported()) after creation and returns wrapped error on auth failure.
Tool Client Initialization Updates
src/pkg/agent/tools/deploy.go, destroy.go, estimate.go, listConfig.go, logs.go, removeConfig.go, services.go, setConfig.go
All tools updated to call GetClientWithRetry(ctx, cli, sc.FabricAddr) instead of passing entire sc config.
Tool Output Messages with Stack Context
src/pkg/agent/tools/listConfig.go, removeConfig.go, setConfig.go
Tool responses now include stack name (sc.Stack.Name) alongside project name in success/error messages.
New Project Name Tool
src/pkg/agent/tools/project_name.go
Adds ProjectNameParams and HandleProjectNameTool to load project name via client.Loader interface.
Tool Registry and Descriptions
src/pkg/agent/tools/tools.go
Updates tool descriptions to clarify stack-scoped operations, adjusts CLI variable wiring, and registers new project_name tool.
Test Mocks and Assertions
src/pkg/agent/tools/deploy_test.go, destroy_test.go, listConfig_test.go, removeConfig_test.go, setConfig_test.go, src/pkg/cli/client/mock.go
Mock providers return non-nil client.MockProvider{}, add Authenticate method, and test assertions expect stack context in output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • DefangLabs/defang#1949: Switches connection/auth flow from cluster config to fabricAddr in GetClientWithRetry and call sites.
  • DefangLabs/defang#1721: Modifies provider setup flow in src/pkg/agent/tools/provider.go with stack selection and provider authentication.
  • DefangLabs/defang#1687: Part of larger Defang Agent refactor that introduced StackConfig and updated tools to pass stack/fabric address.

Suggested reviewers

  • jordanstephens
  • edwardrf

Poem

🐰 A rabbit hops through fabric threads so fine,
No longer configs passed in every line—
Just addresses flow, authentication blooms,
Stack names in messages light up the rooms!
The project name tool joins the merry quest,
Refactored with care, now tested and blessed! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: updating GetClientWithRetry to use fabricAddr and enhancing error messages/behavior for stack context.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lio/fix-agent

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."
level=warning msg="Suggested new configuration:\nlinters:\n enable:\n - gomodguard_v2\n"
level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between af54c0c and 5087e21.

📒 Files selected for processing (18)
  • src/pkg/agent/tools/auth.go
  • src/pkg/agent/tools/deploy.go
  • src/pkg/agent/tools/deploy_test.go
  • src/pkg/agent/tools/destroy.go
  • src/pkg/agent/tools/destroy_test.go
  • src/pkg/agent/tools/estimate.go
  • src/pkg/agent/tools/listConfig.go
  • src/pkg/agent/tools/listConfig_test.go
  • src/pkg/agent/tools/logs.go
  • src/pkg/agent/tools/project_name.go
  • src/pkg/agent/tools/provider.go
  • src/pkg/agent/tools/removeConfig.go
  • src/pkg/agent/tools/removeConfig_test.go
  • src/pkg/agent/tools/services.go
  • src/pkg/agent/tools/setConfig.go
  • src/pkg/agent/tools/setConfig_test.go
  • src/pkg/agent/tools/tools.go
  • src/pkg/cli/client/mock.go

Comment on lines +10 to 22
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
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

Comment on lines +15 to +19
func HandleProjectNameTool(ctx context.Context, loader client.Loader) (string, error) {
pn, _, err := loader.LoadProjectName(ctx)
if err != nil {
return "", err
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

Comment on lines +53 to +55
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)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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.

Comment on lines +39 to +41
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{}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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.

Comment on lines +139 to +142
loader, err := common.ConfigureAgentLoader(params.LoaderParams)
if err != nil {
return "Failed to configure loader", err
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.go

Repository: DefangLabs/defang

Length of output: 505


🏁 Script executed:

head -20 src/pkg/agent/tools/tools.go

Repository: DefangLabs/defang

Length of output: 778


🏁 Script executed:

sed -n '130,150p' src/pkg/agent/tools/tools.go

Repository: 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.

Suggested change
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).

@lionello lionello merged commit 8ec5b52 into main May 8, 2026
14 checks passed
@lionello lionello deleted the lio/fix-agent branch May 8, 2026 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants