Conversation
This reverts commit 98a5212.
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant LoadFB as LoadProjectNameWithFallback
participant Loader as Local Loader
participant Provider as Provider
participant Remote as RemoteProjectName
CLI->>LoadFB: LoadProjectNameWithFallback(ctx, loader, provider)
LoadFB->>Loader: LoadProjectName(ctx)
alt Local Load Succeeds
Loader-->>LoadFB: projectName, nil
LoadFB-->>CLI: projectName, nil
else Local Load Fails
Loader-->>LoadFB: "", error
LoadFB->>Provider: RemoteProjectName(ctx)
Provider->>Remote: Retrieve remote project
alt Remote Succeeds
Remote-->>Provider: projectName, nil
Provider-->>LoadFB: projectName, nil
LoadFB-->>CLI: projectName, nil
else Remote Fails
Remote-->>Provider: "", error
Provider-->>LoadFB: "", error
LoadFB-->>CLI: "", wrappedError
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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.5.0)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" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pkg/cli/client/projectName.go`:
- Around line 11-26: LoadProjectNameWithFallback currently guards loader but not
provider, causing a possible nil dereference when loader fails and provider is
nil; after the loader block (after the term.Debug("Trying to get the remote
project name from the provider") line) add a nil check for provider and return a
wrapped error (including loadErr) if provider is nil, e.g. return "",
fmt.Errorf("%w and %w", loadErr, fmt.Errorf("provider is nil")), ensuring calls
to provider.RemoteProjectName(ctx) only occur when provider != nil.
🧹 Nitpick comments (3)
src/pkg/agent/tools/setConfig_test.go (1)
67-76: LGTM!The mock method signature correctly implements the updated interface.
Note: The struct fields
LoadProjectNameError(line 20) andLoadProjectNameCalled(line 25) weren't renamed to match the new method name (unlikeMockDestroyCLIwhich usesLoadProjectNameWithFallbackError). This is a minor naming inconsistency that doesn't affect functionality.Optional: Rename fields for consistency with other mocks
type MockSetConfigCLI struct { CLIInterface ConnectError error - LoadProjectNameError error + LoadProjectNameWithFallbackError error ConfigSetError error InteractiveLoginMCPError error ConnectCalled bool NewProviderCalled bool - LoadProjectNameCalled bool + LoadProjectNameWithFallbackCalled boolAnd update references in the test assertions accordingly.
src/pkg/cli/client/projectName.go (1)
23-25: Error message may include<nil>when loader is nil.When
loaderis nil,loadErrremains nil, and the error message on line 24 will read"<nil> and <remote error>". Consider handling this case for cleaner error messages.♻️ Proposed fix for cleaner error handling
projectName, err := provider.RemoteProjectName(ctx) if err != nil { + if loadErr != nil { + return "", fmt.Errorf("%w and %w", loadErr, err) + } - return "", fmt.Errorf("%w and %w", loadErr, err) + return "", err } return projectName, nilsrc/pkg/cli/client/projectName_test.go (1)
11-52: Good test coverage, consider adding edge case.The tests cover the main scenarios well. Consider adding a test case where
loaderis nil to verify the function correctly falls back to the remote provider when no loader is provided.💡 Suggested additional test case
t.Run("nil loader, with fallback", func(t *testing.T) { provider := mockRemoteProjectName{ ProjectName: "remote-project", } projectName, err := LoadProjectNameWithFallback(ctx, nil, provider) if err != nil { t.Fatalf("expected no error, got %v", err) } if projectName != "remote-project" { t.Fatalf("expected project name 'remote-project', got %q", projectName) } })
| func LoadProjectNameWithFallback(ctx context.Context, loader Loader, provider Provider) (string, error) { | ||
| var loadErr error | ||
| if loader != nil { | ||
| projectName, _, err := loader.LoadProjectName(ctx) | ||
| if err == nil { | ||
| return projectName, nil | ||
| } | ||
| term.Debug("Failed to load local project:", err) | ||
| loadErr = err | ||
| } | ||
| term.Debug("Trying to get the remote project name from the provider") | ||
| projectName, err := provider.RemoteProjectName(ctx) | ||
| if err != nil { | ||
| return "", fmt.Errorf("%w and %w", loadErr, err) | ||
| } | ||
| return projectName, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to LoadProjectNameWithFallback
rg -n 'LoadProjectNameWithFallback' --type goRepository: DefangLabs/defang
Length of output: 5526
🏁 Script executed:
#!/bin/bash
# Read the test file to see what's expected when provider is nil
cat -n src/pkg/cli/client/projectName_test.goRepository: DefangLabs/defang
Length of output: 2258
🏁 Script executed:
#!/bin/bash
# Find the session type definition to check if Provider can be nil
rg -n 'type.*Session.*struct' --type go -A 10Repository: DefangLabs/defang
Length of output: 1972
🏁 Script executed:
#!/bin/bash
# Read the full newCommandSessionWithOpts function to see how Session is created
sed -n '31,100p' src/cmd/cli/command/session.goRepository: DefangLabs/defang
Length of output: 2585
🏁 Script executed:
#!/bin/bash
# Check the LoadSession method to see if Provider can be nil
rg -n 'func.*LoadSession' --type go -A 20 | head -40Repository: DefangLabs/defang
Length of output: 2458
🏁 Script executed:
#!/bin/bash
# Double-check by looking at how Provider interface is defined
rg -n 'type Provider interface' --type go -A 10Repository: DefangLabs/defang
Length of output: 1123
Add nil check for provider parameter to prevent panic.
The function checks if loader is nil (line 13) but doesn't check if provider is nil before calling provider.RemoteProjectName(ctx) on line 22. Although the test file explicitly passes a nil provider (when the loader succeeds and returns early), there's no test coverage for the case where both the loader fails and the provider is nil, which would cause a nil pointer dereference panic. Add a nil check after the loader block for consistency and defensive programming.
Proposed fix
func LoadProjectNameWithFallback(ctx context.Context, loader Loader, provider Provider) (string, error) {
var loadErr error
if loader != nil {
projectName, _, err := loader.LoadProjectName(ctx)
if err == nil {
return projectName, nil
}
term.Debug("Failed to load local project:", err)
loadErr = err
}
+ if provider == nil {
+ return "", loadErr
+ }
term.Debug("Trying to get the remote project name from the provider")
projectName, err := provider.RemoteProjectName(ctx)📝 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 LoadProjectNameWithFallback(ctx context.Context, loader Loader, provider Provider) (string, error) { | |
| var loadErr error | |
| if loader != nil { | |
| projectName, _, err := loader.LoadProjectName(ctx) | |
| if err == nil { | |
| return projectName, nil | |
| } | |
| term.Debug("Failed to load local project:", err) | |
| loadErr = err | |
| } | |
| term.Debug("Trying to get the remote project name from the provider") | |
| projectName, err := provider.RemoteProjectName(ctx) | |
| if err != nil { | |
| return "", fmt.Errorf("%w and %w", loadErr, err) | |
| } | |
| return projectName, nil | |
| func LoadProjectNameWithFallback(ctx context.Context, loader Loader, provider Provider) (string, error) { | |
| var loadErr error | |
| if loader != nil { | |
| projectName, _, err := loader.LoadProjectName(ctx) | |
| if err == nil { | |
| return projectName, nil | |
| } | |
| term.Debug("Failed to load local project:", err) | |
| loadErr = err | |
| } | |
| if provider == nil { | |
| return "", loadErr | |
| } | |
| term.Debug("Trying to get the remote project name from the provider") | |
| projectName, err := provider.RemoteProjectName(ctx) | |
| if err != nil { | |
| return "", fmt.Errorf("%w and %w", loadErr, err) | |
| } | |
| return projectName, nil | |
| } |
🤖 Prompt for AI Agents
In `@src/pkg/cli/client/projectName.go` around lines 11 - 26,
LoadProjectNameWithFallback currently guards loader but not provider, causing a
possible nil dereference when loader fails and provider is nil; after the loader
block (after the term.Debug("Trying to get the remote project name from the
provider") line) add a nil check for provider and return a wrapped error
(including loadErr) if provider is nil, e.g. return "", fmt.Errorf("%w and %w",
loadErr, fmt.Errorf("provider is nil")), ensuring calls to
provider.RemoteProjectName(ctx) only occur when provider != nil.
1 participant
Reverts #1919
defang pswithout add'l info should imply Playgrounddefang ps -Pdefangshould show deployed projectStep 2. was already broken in prev CLI release. Step 3. broke with #1919.
Summary by CodeRabbit