Defang Agent #1687
Defang Agent #1687
Conversation
jordanstephens
requested review from
KevyVo,
Copilot and
lionello
and removed request for
lionello
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pkg/cli/getServices.go (1)
82-109: Fix goroutine closure ofendpointand add synchronization for concurrent state updatesThe log message change is fine, but the surrounding goroutine logic has two issues:
- The goroutine closes over the loop variable
endpointwithout capturing it. While Go 1.24 correctly scopes loop variables per iteration, explicitly passingendpointas a parameter is clearer and more defensive:- go func(serviceInfo *defangv1.ServiceInfo) { + go func(serviceInfo *defangv1.ServiceInfo, endpoint string) { defer wg.Done() url := "https://" + endpoint + serviceInfo.HealthcheckPath // Use the regular net/http package to make the request without retries req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) @@ - } - }(serviceInfo) + } + }(serviceInfo, endpoint)
- Multiple goroutines can write
serviceInfo.Stateconcurrently without synchronization. For services with multiple endpoints, whichever goroutine finishes last determines the final state, leading to nondeterministic behavior and potential race detector warnings. Add a mutex or use a channel to aggregate health check results per service before settingServiceInfo.Stateonce with a defined policy (e.g., "healthy if any endpoint returns 2xx").src/pkg/agent/tools/logs.go (1)
75-78: Duplicate error wrapping.The error is wrapped twice with the same message "failed to fetch logs". Remove the redundant wrapping.
if err != nil { - err = fmt.Errorf("failed to fetch logs: %w", err) - term.Error("Failed to fetch logs", "error", err) - return "", fmt.Errorf("failed to fetch logs: %w", err) + term.Error("Failed to fetch logs", "error", err) + return "", fmt.Errorf("failed to fetch logs: %w", err) }src/pkg/agent/tools/removeConfig_test.go (1)
132-140: Remove unused variableargs.The
argsmap is created and populated but never used. This appears to be leftover code from a previous implementation.- // Create request - args := map[string]interface{}{} - if tt.configName != "" { - args["name"] = tt.configName - } - params := RemoveConfigParams{ Name: tt.configName, }
♻️ Duplicate comments (6)
src/pkg/debug/debug.go (1)
43-48: Nil pointer dereference:dc.Projectaccessed before nil check.Line 43 accesses
dc.Project.WorkingDirbefore the nil check on line 46. Ifdc.Projectis nil, this will panic.Apply this diff to fix the nil pointer dereference:
- if dc.Project.WorkingDir != "" { - cmd += " --cwd=" + dc.Project.WorkingDir - } if dc.Project != nil { + if dc.Project.WorkingDir != "" { + cmd += " --cwd=" + dc.Project.WorkingDir + } cmd += " --project-name=" + dc.Project.Name }src/pkg/elicitations/elicitations.go (1)
8-12: Interface parameter names improve discoverability.Adding parameter names to the interface methods (e.g.,
message,field,defaultValue,options) would improve readability when viewing the interface without needing to inspect implementations. This is a stylistic improvement and not blocking.type Controller interface { - RequestString(ctx context.Context, message, field string) (string, error) - RequestStringWithDefault(ctx context.Context, message, field, defaultValue string) (string, error) - RequestEnum(ctx context.Context, message, field string, options []string) (string, error) + RequestString(ctx context.Context, message string, field string) (string, error) + RequestStringWithDefault(ctx context.Context, message string, field string, defaultValue string) (string, error) + RequestEnum(ctx context.Context, message string, field string, options []string) (string, error) }src/pkg/agent/plugins/compat_oai/generate.go (1)
286-286: Token usage still not captured correctly in streaming mode.The OpenAI streaming API sends usage data in a final chunk with an empty
choicesarray. The current code skips this chunk (line 286:if len(chunk.Choices) > 0) and accumulates usage from intermediate chunks which haveusage: null. Additionally, usage is only reported whenstream_options: {"include_usage": true}is set in the request.To fix:
- Add
StreamOptionswithIncludeUsage: trueto the request.- Handle the final usage chunk outside the
len(chunk.Choices) > 0block.for stream.Next() { chunk := stream.Current() + // Capture usage from final chunk (has empty choices) + if chunk.Usage.TotalTokens > 0 { + fullResponse.Usage.InputTokens = int(chunk.Usage.PromptTokens) + fullResponse.Usage.OutputTokens = int(chunk.Usage.CompletionTokens) + fullResponse.Usage.TotalTokens = int(chunk.Usage.TotalTokens) + } if len(chunk.Choices) > 0 { // ... existing choice handling ... - fullResponse.Usage.InputTokens += int(chunk.Usage.PromptTokens) - fullResponse.Usage.OutputTokens += int(chunk.Usage.CompletionTokens) - fullResponse.Usage.TotalTokens += int(chunk.Usage.TotalTokens) } }Also applies to: 368-370
src/pkg/elicitations/survey.go (1)
98-113: The enum type handling now correctly supports both[]stringand[]any.This addresses the prior review feedback about JSON unmarshaling yielding
[]anyinstead of[]string.src/pkg/agent/tools/destroy.go (1)
37-41: The nil check forconfig.ProviderIDaddresses the prior review concern.This prevents the potential nil pointer dereference that was flagged previously.
src/pkg/agent/tools/removeConfig_test.go (1)
120-125: Environment variable cleanup may affect parallel tests.Same issue as in
setConfig_test.go. Uset.Setenvinstead ofos.Unsetenvfor safer parallel test execution.t.Run(tt.name, func(t *testing.T) { t.Chdir("testdata") - os.Unsetenv("DEFANG_PROVIDER") - os.Unsetenv("AWS_PROFILE") - os.Unsetenv("AWS_REGION") + t.Setenv("DEFANG_PROVIDER", "") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_REGION", "")
🧹 Nitpick comments (14)
src/pkg/debug/debug.go (1)
163-199: Considerstrings.Builderfor string concatenation.While the current implementation works correctly, using
strings.Builderwould be more efficient for building the prompt with multiple concatenations. However, since this function is called infrequently (only on deployment errors), the performance impact is negligible.Based on learnings from past comments.
Example refactor:
func buildDeploymentDebugPrompt(debugConfig DebugConfig) string { var sb strings.Builder sb.WriteString("An error occurred while deploying this project") if debugConfig.ProviderID == nil { sb.WriteString(" with Defang.") } else { sb.WriteString(fmt.Sprintf(" to %s with Defang.", debugConfig.ProviderID.Name())) } sb.WriteString(" Help troubleshoot and recommend a solution. Look at the logs to understand what happened.") // ... rest of the string building return sb.String() }src/pkg/elicitations/elicitations.go (1)
75-78: Error message could be more descriptive.When the type assertion fails, the error "invalid %s value" doesn't indicate what type was received. Consider including the actual type for debugging.
value, ok := response.Content[field].(string) if !ok { - return "", fmt.Errorf("invalid %s value", field) + return "", fmt.Errorf("invalid %s value: expected string, got %T", field, response.Content[field]) }src/pkg/agent/plugins/compat_oai/generate.go (1)
244-251: Consider usingstrings.Builderfor content concatenation.The current implementation uses string concatenation in a loop, which creates new string allocations on each iteration. For messages with many parts, using
strings.Builderwould be more efficient.+import "strings" + // concatenateContent concatenates text content into a single string func (g *ModelGenerator) concatenateContent(parts []*ai.Part) string { - content := "" + var sb strings.Builder for _, part := range parts { - content += part.Text + sb.WriteString(part.Text) } - return content + return sb.String() }src/pkg/agent/tools/listConfig_test.go (1)
117-119: Uset.Setenvfor automatic cleanup of environment variables.
os.Unsetenvdoesn't restore original values after the test, which can cause test pollution, especially with parallel execution. Uset.Setenv("VAR", "")or save and defer restoration for proper isolation.- os.Unsetenv("DEFANG_PROVIDER") - os.Unsetenv("AWS_PROFILE") - os.Unsetenv("AWS_REGION") + t.Setenv("DEFANG_PROVIDER", "") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_REGION", "")Note:
t.Setenvautomatically restores the original value when the test completes.src/pkg/elicitations/survey.go (3)
27-28: Remove unnecessary capacity argument for map initialization.
make(map[string]any, 0)is equivalent tomake(map[string]any). The zero capacity is superfluous.- var response = make(map[string]any, 0) + var response = make(map[string]any)
48-48: Same here: remove the unnecessary zero capacity.- content := make(map[string]any, 0) + content := make(map[string]any)
121-132: Simplify by removing the unnecessaryelseafter the early return.Since the
ifblock returns, theelseis redundant.return &survey.Question{ Name: key, Prompt: &survey.Select{ Message: description, Options: options, }, }, nil - } else { - inputPrompt := &survey.Input{ - Message: description, - } - if defaultValue, ok := propMap["default"].(string); ok { - inputPrompt.Default = defaultValue - } - return &survey.Question{ - Name: key, - Prompt: inputPrompt, - }, nil } + inputPrompt := &survey.Input{ + Message: description, + } + if defaultValue, ok := propMap["default"].(string); ok { + inputPrompt.Default = defaultValue + } + return &survey.Question{ + Name: key, + Prompt: inputPrompt, + }, nil }src/pkg/agent/tools/services.go (1)
57-65: Consider logging the JSON marshalling error.When
json.Marshalfails, the error is silently discarded. While the fallback message is returned, the actual error could aid debugging.// Convert to JSON jsonData, jsonErr := json.Marshal(serviceResponse) if jsonErr == nil { term.Debugf("Successfully loaded services with count: %d", len(serviceResponse)) return string(jsonData) + "\nIf you would like to see more details about your deployed projects, please visit the Defang portal at https://portal.defang.io/projects", nil } + term.Debug("Failed to marshal services to JSON:", jsonErr) // Return the data in a structured format return "Successfully loaded services, but failed to convert to JSON. Please check the logs for details.", nilsrc/pkg/agent/tools/setConfig.go (2)
20-31: Inconsistent error message capitalization.Go convention is to use lowercase error messages. Line 24 uses "Could not connect" while line 30 uses "failed to setup provider". Standardize to lowercase for consistency.
if err != nil { - return "", fmt.Errorf("Could not connect: %w", err) + return "", fmt.Errorf("could not connect: %w", err) }
42-44: Inconsistent error message capitalization.Line 43 uses "Invalid config name" (capitalized) while other errors in this function use lowercase. Standardize for consistency.
if !pkg.IsValidSecretName(params.Name) { - return "", fmt.Errorf("Invalid config name: secret name %q is not valid", params.Name) + return "", fmt.Errorf("invalid config name: secret name %q is not valid", params.Name) }src/pkg/agent/tools/removeConfig.go (1)
20-31: Inconsistent error message capitalization.Same issue as in
setConfig.go—Line 24 uses "Could not connect" (capitalized) while other errors use lowercase.if err != nil { - return "", fmt.Errorf("Could not connect: %w", err) + return "", fmt.Errorf("could not connect: %w", err) }src/pkg/agent/tools/setConfig_test.go (1)
221-226: Environment variable cleanup may affect parallel tests.Using
os.Unsetenvwithout saving and restoring original values can cause flaky behavior in parallel test runs. Consider usingt.Setenvwhich automatically restores values after the test.t.Run(tt.name, func(t *testing.T) { t.Chdir("testdata") - os.Unsetenv("DEFANG_PROVIDER") - os.Unsetenv("AWS_PROFILE") - os.Unsetenv("AWS_REGION") + t.Setenv("DEFANG_PROVIDER", "") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_REGION", "")Note:
t.Setenvsets the variable and automatically restores it after the test completes, which is safer for parallel test execution.src/pkg/agent/tools/tools.go (1)
14-21: Inconsistent variable declaration style.The
servicestool handler uses explicit type annotation (var cli CLIInterface = &DefaultToolCLI{}), while all other handlers use short declaration (cli := &DefaultToolCLI{}). Standardize for consistency.- var cli CLIInterface = &DefaultToolCLI{} + cli := &DefaultToolCLI{} return HandleServicesTool(ctx.Context, loader, cli, ec, config)src/pkg/agent/toolmanager.go (1)
63-67: Consider more specific response identifiers.The loop-detection error uses generic
Name: "error"andRef: "error". If the agent or downstream logic needs to distinguish this specific error case (infinite loop prevention) from other tool errors, consider using more specific identifiers likeName: "loop_detected"orRef: "loop_prevention".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/cmd/cli/command/compose.go(7 hunks)src/pkg/agent/common/common.go(2 hunks)src/pkg/agent/plugins/compat_oai/generate.go(1 hunks)src/pkg/agent/toolmanager.go(1 hunks)src/pkg/agent/toolmanager_test.go(1 hunks)src/pkg/agent/tools/destroy.go(1 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/listConfig_test.go(5 hunks)src/pkg/agent/tools/logs.go(2 hunks)src/pkg/agent/tools/provider.go(1 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/removeConfig_test.go(6 hunks)src/pkg/agent/tools/services.go(2 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/agent/tools/setConfig_test.go(5 hunks)src/pkg/agent/tools/tools.go(1 hunks)src/pkg/cli/debug.go(0 hunks)src/pkg/cli/debug_test.go(0 hunks)src/pkg/cli/getServices.go(1 hunks)src/pkg/debug/debug.go(1 hunks)src/pkg/debug/debug_test.go(1 hunks)src/pkg/elicitations/elicitations.go(1 hunks)src/pkg/elicitations/survey.go(1 hunks)src/pkg/elicitations/survey_test.go(1 hunks)src/pkg/mcp/tools/tools.go(1 hunks)src/pkg/migrate/heroku.go(1 hunks)src/pkg/utils.go(1 hunks)
💤 Files with no reviewable changes (2)
- src/pkg/cli/debug.go
- src/pkg/cli/debug_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pkg/debug/debug_test.go
- src/pkg/agent/tools/provider.go
- src/pkg/mcp/tools/tools.go
🧰 Additional context used
🧬 Code graph analysis (15)
src/pkg/agent/tools/services.go (6)
src/pkg/agent/common/common.go (1)
LoaderParams(25-29)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(20-24)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)
src/pkg/agent/tools/setConfig_test.go (3)
src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/elicitations/elicitations.go (1)
NewController(32-36)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)
src/cmd/cli/command/compose.go (2)
src/pkg/debug/debug.go (4)
NewDebugger(75-84)DebugConfig(22-30)Debugger(70-73)P(20-20)src/pkg/track/track.go (2)
Evt(37-52)P(19-21)
src/pkg/agent/tools/listConfig.go (7)
src/pkg/cli/client/client.go (1)
ProjectLoader(12-15)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(20-24)src/pkg/mcp/mcp_server.go (1)
StackConfig(48-48)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)
src/pkg/agent/tools/removeConfig.go (8)
src/pkg/agent/common/common.go (1)
LoaderParams(25-29)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(17-36)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(20-24)src/pkg/cli/connect.go (1)
Connect(16-33)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/cli/configDelete.go (1)
ConfigDelete(12-20)
src/pkg/agent/tools/logs.go (7)
src/pkg/agent/common/common.go (1)
LoaderParams(25-29)src/pkg/cli/client/client.go (1)
ProjectLoader(12-15)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/timeutils/timeutils.go (1)
ParseTimeOrDuration(12-37)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/logs/log_type.go (2)
LogType(8-8)LogTypeAll(23-23)
src/pkg/agent/toolmanager_test.go (1)
src/pkg/agent/toolmanager.go (1)
ToolManager(38-43)
src/pkg/agent/tools/setConfig.go (8)
src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/clouds/driver.go (1)
ProjectName(8-8)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/utils.go (1)
IsValidSecretName(43-45)src/pkg/cli/configSet.go (1)
ConfigSet(12-20)src/cmd/cli/command/commands.go (1)
updateProviderID(1216-1270)
src/pkg/agent/tools/removeConfig_test.go (6)
src/pkg/agent/tools/removeConfig.go (2)
RemoveConfigParams(14-17)HandleRemoveConfigTool(20-46)src/pkg/cli/client/mock.go (1)
MockLoader(162-165)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/elicitations/elicitations.go (1)
NewController(32-36)src/pkg/cli/client/provider_id.go (2)
ProviderAWS(15-15)ProviderID(10-10)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)
src/pkg/elicitations/survey.go (2)
src/pkg/term/colorizer.go (2)
FileReader(42-45)FileWriter(47-50)src/pkg/elicitations/elicitations.go (2)
Request(22-25)Response(27-30)
src/pkg/agent/toolmanager.go (4)
src/pkg/agent/printer.go (1)
Printer(12-15)src/pkg/term/colorizer.go (5)
Printf(286-288)Error(318-320)Println(282-284)Errorf(322-324)Debugf(298-300)src/pkg/agent/tee.go (1)
TeeTerm(16-18)src/pkg/agent/common/common.go (1)
ErrNoProviderSet(16-16)
src/pkg/agent/tools/listConfig_test.go (4)
src/pkg/elicitations/elicitations.go (1)
NewController(32-36)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)src/pkg/mcp/mcp_server.go (1)
StackConfig(48-48)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)
src/pkg/agent/common/common.go (1)
src/pkg/cli/compose/loader.go (2)
NewLoader(57-72)WithPath(45-49)
src/pkg/agent/tools/destroy.go (7)
src/pkg/agent/common/common.go (1)
LoaderParams(25-29)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(20-24)src/pkg/cli/connect.go (1)
Connect(16-33)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)
src/pkg/debug/debug.go (7)
src/cmd/cli/command/commands.go (1)
P(50-50)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/term/colorizer.go (2)
Error(318-320)DefaultTerm(27-27)src/pkg/track/track.go (1)
Evt(37-52)src/pkg/utils.go (1)
IsValidTime(141-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (34)
src/pkg/utils.go (1)
156-156: LGTM: Error message follows Go conventions.The change to lowercase the error message aligns with Go's style guidelines, which recommend that error strings should not be capitalized unless they begin with proper nouns or acronyms.
src/pkg/migrate/heroku.go (1)
279-279: LGTM! Follows Go error message conventions.The change to lowercase "failed" aligns with Go's style guidelines that error strings should not be capitalized unless they begin with proper nouns or acronyms.
src/cmd/cli/command/compose.go (2)
176-180: Correct error handling pattern.This properly preserves the original
deploymentErrwhen debugger creation fails, warning the user about the debugger failure without losing the deployment error context.
473-477: Correct error handling pattern.This properly preserves the original
loadErrwhen debugger creation fails, consistent with the pattern used at lines 176-180.src/pkg/debug/debug.go (5)
56-64: Clean abstraction for testability.The
Surveyorinterface provides a good abstraction layer for testing, allowing mock implementations to be injected.
75-84: LGTM!The constructor follows a clean pattern with proper error handling.
86-93: LGTM!Proper validation of deployment ID before proceeding with debug session.
109-135: Well-structured orchestration with comprehensive telemetry.The method properly tracks events at each stage of the debug session (prompted, accepted/skipped, feedback) with appropriate error handling.
102-107: Missing space in error message.Line 104 concatenates strings without proper spacing. The message reads "compose file.Help troubleshoot" instead of "compose file. Help troubleshoot".
Apply this diff to fix the spacing:
- prompt := "The following error occurred while loading the compose file. Help troubleshoot and recommend a solution." + loadErr.Error() + prompt := "The following error occurred while loading the compose file. Help troubleshoot and recommend a solution. " + loadErr.Error()Likely an incorrect or invalid review comment.
src/pkg/agent/plugins/compat_oai/generate.go (1)
119-139: LGTM - User message handling is correctly consolidated.The user message is now built as a single message with both text and media parts, addressing the previous duplicate message issue.
src/pkg/agent/toolmanager_test.go (1)
9-59: LGTM - Comprehensive test coverage for loop detection.The test covers all essential scenarios for
EqualPrevious: initial state, identical requests, modified values, and different lengths. The helper function keeps the test readable.src/pkg/elicitations/survey_test.go (2)
12-103: Well-structured table-driven tests.Good coverage of string types, defaults, enums, error conditions, and missing descriptions. The test structure is clean and maintainable.
51-65: Test case is valid but lacks coverage of JSON-unmarshaled data.The implementation properly handles both
[]string(lines 101-102) and[]any(lines 103-110) enum types via a type switch. The test case using[]stringis correct, but it only covers one branch. Add a test case with[]anyto cover the realistic JSON-unmarshaling scenario where arrays become[]any.src/pkg/agent/tools/listConfig_test.go (2)
114-164: Tests updated correctly for the new API surface.The test properly exercises the new
StackConfig-based API with elicitations controller integration. The mock setup and call sequence verification look correct.
129-134:mockElicitationsClientis defined inservices_test.goin the same package.The type
mockElicitationsClientis defined at line 126 ofsrc/pkg/agent/tools/services_test.go. Since it's in the same package, it's accessible tolistConfig_test.go; no changes are required.src/pkg/agent/tools/listConfig.go (1)
14-30: LGTM! Clean integration of the provider preparation flow.The refactored signature correctly uses
scfor StackConfig (addressing the past review feedback), and the provider preparation flow viaNewProviderPreparerandSetupProvideraligns with the pattern established across other tool handlers.src/pkg/agent/tools/destroy.go (1)
15-30: LGTM! Consistent with the updated tool pattern.The refactored signature, provider preparation flow, and error handling align with the patterns established across other tool handlers.
src/pkg/agent/tools/logs.go (1)
24-39: LGTM! Time parsing logic is well-structured.The
SinceandUntilparameter parsing correctly usestimeutils.ParseTimeOrDurationwith appropriate error messages.src/pkg/agent/tools/services.go (1)
22-33: No action needed forconfig.ProviderIDvalidation in this tool.
HandleServicesTooldoes not useconfig.ProviderIDand obtains the provider throughSetupProvider()instead. Unlikedestroy.go, which explicitly requires and validatesconfig.ProviderIDfor authorization checks viaCanIUseProvider(), the services tool has a different design that doesn't require this validation. This is not an inconsistency but rather a difference in implementation requirements between the two tools.src/pkg/agent/tools/setConfig.go (2)
14-18: LGTM on struct definition.The
SetConfigParamsstruct correctly embedscommon.LoaderParamsand uses proper JSON schema tags for the required fields.
51-51: Grammar issue in success message."Successfully set" is correct here—no issue with this line.
src/pkg/agent/tools/removeConfig.go (1)
14-17: LGTM on struct definition.The
RemoveConfigParamsstruct correctly embedscommon.LoaderParamswith the requiredNamefield.src/pkg/agent/tools/removeConfig_test.go (1)
114-116: Test expectation matches current buggy output.This test expects
"Successfully remove the config variable"which matches the grammar bug inremoveConfig.goLine 45. When that bug is fixed to "Successfully removed", this test will need to be updated accordingly.src/pkg/agent/tools/tools.go (1)
9-100: LGTM on overall structure.The
CollectDefangToolsfunction cleanly defines all agent tools using a consistent pattern. Each tool properly configures its loader and delegates to the appropriate handler with the sharedecandconfigcontext.src/pkg/agent/tools/setConfig_test.go (1)
131-154: Test cases have misleading names that don't match their actual validation.The tests "missing config value" and "empty config value" expect the error
"Invalid config name: secret name \"test-config\" is not valid", which validates the name, not the value. The name "test-config" fails validation because hyphens are not allowed in secret names (pattern:^[A-Za-z_][A-Za-z0-9_]{0,63}$). These tests don't verify value validation at all—there is no value validation inHandleSetConfigor the underlyingConfigSetfunction. Either rename these tests to reflect that they validate the name format, or implement actual value validation if that was the intended behavior.Likely an incorrect or invalid review comment.
src/pkg/agent/toolmanager.go (6)
15-36: LGTM! Clean interface and implementation.The GenkitToolManager interface and its implementation provide a clear abstraction over Genkit tool registration and lookup.
38-52: LGTM! Well-structured manager.The ToolManager aggregates the necessary components for tool execution and loop detection.
54-59: LGTM! Straightforward registration.The method correctly delegates to the underlying manager and maintains local state.
97-109: LGTM! Graceful handling of missing provider.The TeeTerm wrapper captures terminal output during tool execution, and the special handling of
ErrNoProviderSetallows the agent to continue and prompt the user for configuration rather than failing the entire session. This aligns with the PR objective of tools requesting their own configuration.
118-142: LGTM! Loop detection correctly implemented.The set-based equality check correctly compares entire tool request sets rather than triggering on partial overlap. This implementation addresses the past review concern about false positives when the model re-invokes one tool while also requesting a different tool.
121-125: LGTM! Defensive error handling.Continuing after a marshal error is reasonable defensive coding. While it excludes the failed request from loop detection, JSON marshaling of tool inputs should rarely fail, and this prevents the entire comparison from failing.
src/pkg/agent/common/common.go (3)
16-16: LGTM! Clear error message.The error message is concise and clearly communicates the issue.
25-29: LGTM! Well-documented parameter struct.The LoaderParams struct clearly defines the configuration options with helpful JSON schema descriptions.
43-63: LGTM! Clear loader configuration logic.The function correctly prioritizes project name over compose file paths, with a sensible default fallback. The TODO comment (lines 56-60) acknowledges the current limitation of not supporting both parameters simultaneously.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/agent/tools/logs.go (1)
80-84: Remove redundant error wrapping.Lines 81 and 83 both wrap the error with the same message, which is redundant. The error variable on line 81 is assigned but then immediately overwritten on line 83.
Apply this diff to simplify:
if err != nil { - err = fmt.Errorf("failed to fetch logs: %w", err) - term.Error("Failed to fetch logs", "error", err) - return "", fmt.Errorf("failed to fetch logs: %w", err) + term.Error("Failed to fetch logs", "error", err) + return "", fmt.Errorf("failed to fetch logs: %w", err) }
🧹 Nitpick comments (13)
src/pkg/agent/tools/logs.go (2)
29-40: Consider validating the time range.When both
sinceanduntilare provided, the code doesn't verify thatuntilTimeis aftersinceTime. This could lead to confusing behavior if a user provides an invalid time range.Apply this diff to add validation:
if params.Until != "" { untilTime, err = timeutils.ParseTimeOrDuration(params.Until, now) if err != nil { return "", fmt.Errorf("invalid parameter 'until', must be in RFC3339 format: %w", err) } } + if !sinceTime.IsZero() && !untilTime.IsZero() && untilTime.Before(sinceTime) { + return "", errors.New("'until' time must be after 'since' time") + }
70-78: Consider making the log limit configurable.The limit is hardcoded to 100 entries. For debugging complex issues, users might need more log entries. Consider adding a
limitparameter toLogsParamswith a reasonable default.Add a field to
LogsParams:Limit int `json:"limit,omitempty" jsonschema:"description=Optional: Maximum number of log entries to retrieve (default: 100).,minimum=1,maximum=10000"`Then use it in the Tail call:
+ limit := params.Limit + if limit == 0 { + limit = 100 + } err = cli.Tail(ctx, provider, projectName, cliTypes.TailOptions{ Deployment: params.DeploymentID, Since: sinceTime, Until: untilTime, - Limit: 100, + Limit: limit, LogType: logs.LogTypeAll, PrintBookends: true, Verbose: true, })src/pkg/debug/debug.go (4)
32-54: Consider usingstrings.Builderfor string concatenation.As noted in a previous review comment, when building strings with multiple concatenations,
strings.Builderis more efficient than repeated+=operations. While this method is not in a hot path, the refactor would align with Go best practices.Example refactor:
func (dc DebugConfig) String() string { - cmd := "debug" + var b strings.Builder + b.WriteString("debug") if dc.Deployment != "" { - cmd += " --deployment=" + dc.Deployment + b.WriteString(" --deployment=") + b.WriteString(dc.Deployment) } if !dc.Since.IsZero() { - cmd += " --since=" + dc.Since.UTC().Format(time.RFC3339Nano) + b.WriteString(" --since=") + b.WriteString(dc.Since.UTC().Format(time.RFC3339Nano)) } if !dc.Until.IsZero() { - cmd += " --until=" + dc.Until.UTC().Format(time.RFC3339Nano) + b.WriteString(" --until=") + b.WriteString(dc.Until.UTC().Format(time.RFC3339Nano)) } if dc.Project != nil { - cmd += " --project-name=" + dc.Project.Name + b.WriteString(" --project-name=") + b.WriteString(dc.Project.Name) if dc.Project.WorkingDir != "" { - cmd += " --cwd=" + dc.Project.WorkingDir + b.WriteString(" --cwd=") + b.WriteString(dc.Project.WorkingDir) } } if len(dc.FailedServices) > 0 { - cmd += " " + strings.Join(dc.FailedServices, " ") + b.WriteString(" ") + b.WriteString(strings.Join(dc.FailedServices, " ")) } - // TODO: do we need to add --provider= or rely on the Fabric-supplied value? - return cmd + return b.String() }
137-148: Simplify return statement.Line 147 returns
errwhich has already been checked for non-nil values above, so it will always benilat this point. Consider simplifying.Apply this diff:
func (d *Debugger) promptForPermission() (bool, error) { var aiDebug bool err := d.surveyor.AskOne(&survey.Confirm{ Message: "Would you like to debug the deployment with AI?", Help: "This will send logs and artifacts to our backend and attempt to diagnose the issue and provide a solution.", }, &aiDebug, survey.WithStdio(term.DefaultTerm.Stdio())) if err != nil { return false, err } - return aiDebug, err + return aiDebug, nil }
150-161: Simplify return statement.Line 160 has the same pattern as
promptForPermission: it returnserrwhich will always benilat this point.Apply this diff:
func (d *Debugger) promptForFeedback() (bool, error) { var good bool err := d.surveyor.AskOne(&survey.Confirm{ Message: "Was the debugging helpful?", Help: "Please provide feedback to help us improve the debugging experience.", }, &good, survey.WithStdio(term.DefaultTerm.Stdio())) if err != nil { return false, err } - return good, err + return good, nil }
163-199: Consider usingstrings.Builderfor prompt construction.This function performs many string concatenations to build a potentially large AI prompt. Using
strings.Builderwould be more efficient and aligns with Go best practices, especially since the prompt may include the full compose YAML content.Example refactor:
func buildDeploymentDebugPrompt(debugConfig DebugConfig) string { - prompt := "An error occurred while deploying this project" + var b strings.Builder + b.WriteString("An error occurred while deploying this project") if debugConfig.ProviderID == nil { - prompt += " with Defang." + b.WriteString(" with Defang.") } else { - prompt += fmt.Sprintf(" to %s with Defang.", debugConfig.ProviderID.Name()) + fmt.Fprintf(&b, " to %s with Defang.", debugConfig.ProviderID.Name()) } - prompt += " Help troubleshoot and recommend a solution. Look at the logs to understand what happened." + b.WriteString(" Help troubleshoot and recommend a solution. Look at the logs to understand what happened.") if debugConfig.Deployment != "" { - prompt += fmt.Sprintf(" The deployment ID is %q.", debugConfig.Deployment) + fmt.Fprintf(&b, " The deployment ID is %q.", debugConfig.Deployment) } if len(debugConfig.FailedServices) > 0 { - prompt += fmt.Sprintf(" The services that failed to deploy are: %v.", debugConfig.FailedServices) + fmt.Fprintf(&b, " The services that failed to deploy are: %v.", debugConfig.FailedServices) } if pkg.IsValidTime(debugConfig.Since) { - prompt += fmt.Sprintf(" The deployment started at %s.", debugConfig.Since.String()) + fmt.Fprintf(&b, " The deployment started at %s.", debugConfig.Since.String()) } if pkg.IsValidTime(debugConfig.Until) { - prompt += fmt.Sprintf(" The deployment finished at %s.", debugConfig.Until.String()) + fmt.Fprintf(&b, " The deployment finished at %s.", debugConfig.Until.String()) } if debugConfig.Project != nil { yaml, err := debugConfig.Project.MarshalYAML() if err != nil { term.Println("Failed to marshal compose project to YAML for debug:", err) } - prompt += fmt.Sprintf( + fmt.Fprintf(&b, "The compose files are at %s. The compose file is as follows:\n\n%s", debugConfig.Project.ComposeFiles, yaml, ) } - return prompt + return b.String() }src/pkg/agent/tools/removeConfig_test.go (2)
121-124: Uset.Setenvinstead ofos.Unsetenvfor test isolation.Direct
os.Unsetenvcalls persist after the test completes, potentially affecting other tests. Uset.Setenv("DEFANG_PROVIDER", "")which automatically restores the original value after the test.- os.Unsetenv("DEFANG_PROVIDER") - os.Unsetenv("AWS_PROFILE") - os.Unsetenv("AWS_REGION") + t.Setenv("DEFANG_PROVIDER", "") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_REGION", "")
133-136: Remove unusedargsvariable.The
argsmap is created and populated but never used. The test correctly uses theparamsstruct directly.- // Create request - args := map[string]interface{}{} - if tt.configName != "" { - args["name"] = tt.configName - } - params := RemoveConfigParams{src/pkg/agent/tools/tools.go (1)
23-66: Consider extracting common tool setup logic (optional).All tools follow the same pattern: configure loader → create CLI → call handler. While readable, this could be DRY-ed up with a helper. However, given the simplicity and the benefit of explicit code, the current approach is acceptable.
src/pkg/agent/plugins/compat_oai/generate.go (4)
1-16: License notice placement.The package declaration should come before the license header comment in Go files. Consider moving the license comment after the package declaration or using a separate LICENSE file.
46-48: Remove or implement unusedtoolChoicefield.The
toolChoicefield is defined but never used (marked withnolint:unused). Either implement tool choice functionality or remove the field to reduce confusion.tools []openai.ChatCompletionToolParam - // nolint:unused - toolChoice openai.ChatCompletionToolChoiceOptionUnionParam // Store any errors that occur during building err error
246-252: Consider usingstrings.Builderfor content concatenation.String concatenation with
+=in a loop creates a new string on each iteration. For messages with many parts,strings.Builderwould be more efficient.+import "strings" + // concatenateContent concatenates text content into a single string func (g *ModelGenerator) concatenateContent(parts []*ai.Part) string { - content := "" + var sb strings.Builder for _, part := range parts { - content += part.Text + sb.WriteString(part.Text) } - return content + return sb.String() }
485-491: Be cautious with including raw JSON in error messages.The error message includes the raw
jsonStringwhich could potentially contain sensitive data if tool arguments ever include secrets. Consider truncating or omitting the raw value in production error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/pkg/agent/plugins/compat_oai/generate.go(1 hunks)src/pkg/agent/tools/logs.go(2 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/removeConfig_test.go(5 hunks)src/pkg/agent/tools/tools.go(1 hunks)src/pkg/debug/debug.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/pkg/agent/tools/removeConfig.go (7)
src/pkg/agent/tools/interfaces.go (1)
CLIInterface(17-36)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(20-24)src/pkg/cli/connect.go (1)
Connect(16-33)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/cli/configDelete.go (1)
ConfigDelete(12-20)
src/pkg/agent/tools/logs.go (7)
src/pkg/agent/common/common.go (1)
LoaderParams(25-29)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)src/pkg/mcp/mcp_server.go (1)
StackConfig(48-48)src/pkg/timeutils/timeutils.go (1)
ParseTimeOrDuration(12-37)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(30-36)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/logs/log_type.go (2)
LogType(8-8)LogTypeAll(23-23)
src/pkg/agent/tools/removeConfig_test.go (6)
src/pkg/agent/tools/removeConfig.go (2)
RemoveConfigParams(14-17)HandleRemoveConfigTool(20-46)src/pkg/cli/client/mock.go (1)
MockLoader(162-165)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/elicitations/elicitations.go (1)
NewController(32-36)src/pkg/cli/client/provider_id.go (2)
ProviderAWS(15-15)ProviderID(10-10)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-18)
src/pkg/debug/debug.go (6)
src/cmd/cli/command/commands.go (1)
P(50-50)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/track/track.go (1)
Evt(37-52)src/pkg/utils.go (1)
IsValidTime(141-146)
src/pkg/agent/plugins/compat_oai/generate.go (1)
src/pkg/term/colorizer.go (2)
Errorf(322-324)Debugf(298-300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (18)
src/pkg/agent/tools/logs.go (3)
1-23: LGTM!The imports are appropriate and the
LogsParamsstructure is well-defined with clear jsonschema descriptions for each field.
42-52: LGTM!The client connection and provider setup logic is clean with appropriate error handling.
54-63: LGTM! Past review comment addressed.The nil check for
config.ProviderID(lines 61-63) correctly addresses the critical issue flagged in the past review. The project name loading logic is also correct with appropriate error handling.src/pkg/debug/debug.go (6)
43-48: Nil pointer dereference fixed.The previous review concern about accessing
dc.Projectbefore the nil check has been resolved. The code now correctly checksdc.Project != nilbefore accessing its fields.
56-64: LGTM!The
Surveyorinterface provides a clean abstraction over the survey package, enabling easy mocking in tests while maintaining a simple implementation.
66-73: LGTM!The interface-based design enables testability and follows Go best practices for dependency injection.
75-84: LGTM!The constructor properly handles agent initialization errors and returns a well-formed Debugger instance.
86-93: LGTM!Proper validation of the deployment ID and clean integration with the telemetry system. As previously noted, the etag format is acceptable for metadata tracking.
109-135: LGTM!Comprehensive telemetry tracking and clean orchestration of the debug session flow with proper error propagation.
src/pkg/agent/tools/removeConfig_test.go (1)
143-158: LGTM!The test setup correctly constructs the loader, elicitation controller, and StackConfig with appropriate mock values. The test properly exercises the new
HandleRemoveConfigToolsignature with the provider preparer flow.src/pkg/agent/tools/removeConfig.go (3)
14-17: LGTM!The
RemoveConfigParamsstruct correctly embedscommon.LoaderParamsand defines the requiredNamefield with appropriate JSON schema tags.
27-31: LGTM!The provider preparer integration correctly sets up the provider via
SetupProvider. Discarding the first return value (providerId) with_is appropriate since only the provider is needed for the subsequent operations.
37-45: LGTM!The error handling correctly distinguishes between "not found" (returns a message, not an error) and other errors. The success message grammar has been corrected to use past tense.
src/pkg/agent/tools/tools.go (2)
9-22: LGTM!The
CollectDefangToolsfunction properly creates AI tools with consistent patterns. Each tool correctly configures the loader from params, instantiates aDefaultToolCLI, and delegates to the appropriate handler with the shared elicitation controller and stack config.
56-57: Typo has been corrected.The description now correctly reads "Estimate the cost of deploying" instead of "deployed".
src/pkg/agent/plugins/compat_oai/generate.go (3)
120-140: User message handling is correct.The implementation now properly combines text and media content into a single parts array, avoiding the duplicate message issue from the previous review.
401-404: Empty choices check has been added.The guard against empty
Choicesslice prevents a potential panic, addressing the concern from the previous review.
39-67: Well-designed fluent builder pattern.The
ModelGeneratorimplementation provides a clean fluent API for building OpenAI requests. The builder pattern with error accumulation (g.err) allows chaining methods while deferring error handling to theGeneratecall.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pkg/debug/debug.go (2)
32-54: Clarify intent ofDebugConfig.Stringand consider safer CLI representationRight now
String()builds a shell-looking command but:
- It omits
ProviderIDandStack, even though they are part ofDebugConfig.- It concatenates
WorkingDirand service names without quoting, so any value with spaces or shell‑significant chars would not round‑trip as a copy‑pasteable command.If this is meant purely as logging/telemetry metadata, consider making that explicit in a comment or renaming to avoid implying “repro CLI”. If it is intended as a repro command, consider building an argument slice and adding quoting/escaping (or at least quoting
--cwdand service names).
109-135: Track outcome of the debug session (fn) for better telemetry
promptAndTrackDebugSessiontracks prompt/feedback decisions, but the actual outcome offn()(the debug run) is not recorded. Iffn()fails, you just return the error with no event; if it succeeds, there’s no “Completed” event either.Consider emitting explicit success/failure events around
fn()so you can correlate user consent with whether the debug run actually executed and whether it errored:- err = fn() - if err != nil { - return err - } + err = fn() + if err != nil { + track.Evt(eventName+" Failed", append([]track.Property{P("reason", err)}, eventProperty...)...) + return err + } + + track.Evt(eventName+" Completed", eventProperty...)This keeps your analytics consistent with the rest of the flow (prompted / accepted / completed / failed).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pkg/debug/debug.go(1 hunks)src/pkg/debug/debug_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/debug/debug_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
src/pkg/debug/debug.go (8)
src/cmd/cli/command/commands.go (1)
P(50-50)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/term/colorizer.go (3)
Error(318-320)DefaultTerm(27-27)Println(282-284)src/pkg/track/track.go (1)
Evt(37-52)src/pkg/utils.go (1)
IsValidTime(141-146)src/pkg/cli/tail.go (1)
Deployment(46-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
Description
This PR implements a Defang Agent in the CLI. The Defang Agent can be invoked in two ways:
defangat a shell without any subcommand will launch the user into an agent chat on the shell. The user may ask defang to perform desired tasks, and the agent will ask the user for the configuration necessary.On the way to achieving the primary goal of exposing a Defang Agent in the CLI, this PR also does the following:
elicitations.Controllerinterface is used in the cli agent as well, but the implementation uses the survey package instead ofmcp-go.logintool has been removed. Each tool will not automatically check authentication status and open the browser to prompt for sign in when necessary.The mcp server and the defang agent are designed to be oriented around stacks. When a new session is started, defang will ask the user to select a stack or create one. That stack will be used for the remainder of the session.
Linked Issues
Checklist
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.