fix: migrate from term to slog#2064
Open
jordanstephens wants to merge 7 commits intomainfrom
Open
Conversation
Migrated test infrastructure from term to slog to support the slog migration in the CLI. Tests that mock term.DefaultTerm now need to also update the slog default logger to capture output for assertions. Key changes in test files: - Added slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) after replacing term.DefaultTerm to ensure slog output goes to the mock term buffer - Added required imports: log/slog and github.com/DefangLabs/defang/src/pkg/logs - In validation_test.go, renamed 'logs' variable to 'logBuf' to avoid shadowing the logs package Files modified: - cmd/cli/command/compose_test.go - cmd/cli/command/stack_test.go (MockTerm helper) - pkg/cli/client/byoc/aws/byoc_test.go - pkg/cli/compose/compose_test.go - pkg/cli/compose/serviceNameReplacer_test.go - pkg/cli/compose/validation_test.go - pkg/cli/configList_test.go - pkg/cli/configResolution_test.go - pkg/cli/deploymentsList_test.go - pkg/cli/getServices_test.go - pkg/cli/tail_test.go (also fixed TestTail assertion for variable trailing spaces) Also fixed: - Added mutex to termHandler in slog.go to protect against data races when multiple goroutines write concurrently (e.g., in TailAndMonitor) - Fixed TestTail assertion to handle variable number of trailing spaces when clearing the reconnection message (depends on terminal width)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces project-specific Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are primarily a logging backend migration and do not introduce new multi-component control flows requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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.11.4)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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/pkg/cli/getServices.go (1)
112-120:⚠️ Potential issue | 🔴 CriticalProtect shared healthcheck result writes across goroutines.
Line 119 writes to shared per-service status from multiple goroutines without synchronization. If a service has multiple endpoints, this is a data race and can produce nondeterministic health state.
🔧 Suggested fix
func GetHealthcheckResults(ctx context.Context, serviceInfos []*defangv1.ServiceInfo) HealthCheckResults { @@ var wg sync.WaitGroup + var mu sync.Mutex @@ - go func(serviceInfo *defangv1.ServiceInfo) { + go func(serviceName, endpoint, healthcheckPath string) { defer wg.Done() - result, err := RunHealthcheck(ctx, serviceInfo.Service.Name, "https://"+endpoint, serviceInfo.HealthcheckPath) + result, err := RunHealthcheck(ctx, serviceName, "https://"+endpoint, healthcheckPath) if err != nil { - slog.Debug(fmt.Sprintf("Healthcheck error for service %q at endpoint %q: %s", serviceInfo.Service.Name, endpoint, err.Error())) + slog.Debug(fmt.Sprintf("Healthcheck error for service %q at endpoint %q: %s", serviceName, endpoint, err.Error())) result = "error" } - *results[serviceInfo.Service.Name] = result - }(serviceInfo) + mu.Lock() + *results[serviceName] = result + mu.Unlock() + }(serviceInfo.Service.Name, endpoint, serviceInfo.HealthcheckPath) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/getServices.go` around lines 112 - 120, Concurrent goroutines spawned in the anonymous function that calls RunHealthcheck are writing to the shared results map entry via "*results[serviceInfo.Service.Name] = result" without synchronization, causing a data race when a service has multiple endpoints; fix this by protecting writes with a shared sync.Mutex (or sync.RWMutex) declared outside the goroutine and acquire the lock immediately before assigning to results[serviceInfo.Service.Name], then release it afterward, or alternatively make each goroutine send its result back over a per-service channel which a single collector goroutine uses to update results; reference the anonymous goroutine, RunHealthcheck, serviceInfo, and the results map when applying the change.src/pkg/cli/compose/serviceNameReplacer_test.go (1)
122-129:⚠️ Potential issue | 🟡 MinorRestore global slog default logger after each test to prevent state leakage.
Line 126 sets the global
slogdefault logger, but the cleanup function only restoresterm.DefaultTermat line 128. The previous slog logger is never restored, which can cause cross-test interference.Suggested fix
for _, tc := range testCases { prevTerm := term.DefaultTerm + prevLogger := slog.Default() var out, err bytes.Buffer term.DefaultTerm = term.NewTerm(os.Stdin, &out, &err) slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) t.Cleanup(func() { term.DefaultTerm = prevTerm + slog.SetDefault(prevLogger) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/compose/serviceNameReplacer_test.go` around lines 122 - 129, The test sets a new global slog logger with slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) but only restores term.DefaultTerm in the t.Cleanup; capture the previous slog default before calling slog.SetDefault (e.g., prevLogger := slog.Default()) and restore it inside the same t.Cleanup alongside restoring term.DefaultTerm so the original global logger is reinstated after each test; update the cleanup to restore both term.DefaultTerm and the previous slog default (references: term.DefaultTerm, slog.SetDefault, logs.NewTermLogger, t.Cleanup).src/cmd/cli/command/stack_test.go (1)
47-58:⚠️ Potential issue | 🟠 Major
MockTermshould also restore the previousslogdefault logger.This helper mutates global logger state via
slog.SetDefault()but only restoresterm.DefaultTermin cleanup. This leaks logging configuration across tests.🔧 Suggested fix
func MockTerm(t *testing.T, stdout *bytes.Buffer, stdin *bytes.Reader) { t.Helper() oldTerm := term.DefaultTerm + oldLogger := slog.Default() term.DefaultTerm = term.NewTerm( &FakeStdin{stdin}, &FakeStdout{stdout}, new(bytes.Buffer), ) slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) t.Cleanup(func() { term.DefaultTerm = oldTerm + slog.SetDefault(oldLogger) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/stack_test.go` around lines 47 - 58, The MockTerm helper replaces the global slog default logger but only restores term.DefaultTerm; capture the previous slog default before calling slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) (e.g., oldLogger := slog.Default()) and in the t.Cleanup restore both globals by setting term.DefaultTerm = oldTerm and slog.SetDefault(oldLogger); update the MockTerm function to save the prior logger and restore it in the existing cleanup closure alongside term.DefaultTerm.src/cmd/cli/command/compose_test.go (1)
29-37:⚠️ Potential issue | 🟠 MajorRestore the default logger in test cleanup to prevent test isolation issues.
slog.SetDefault()updates global process state. Without restoring the previous logger, this test will affect subsequent tests in the same package, potentially causing flaky test failures.🔧 Suggested fix
defaultTerm := term.DefaultTerm + oldLogger := slog.Default() t.Cleanup(func() { term.DefaultTerm = defaultTerm + slog.SetDefault(oldLogger) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/compose_test.go` around lines 29 - 37, The test sets a new global logger via slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) but only restores term.DefaultTerm in t.Cleanup, leaving the global slog default mutated; capture the previous logger before calling slog.SetDefault (e.g., prev := slog.Default()), then in the existing t.Cleanup restore both term.DefaultTerm and the slog default by calling slog.SetDefault(prev) so the test does not leak global logger state (refer to term.DefaultTerm, term.NewTerm, slog.SetDefault, slog.Default).src/pkg/cli/compose/compose_test.go (1)
187-195:⚠️ Potential issue | 🟠 MajorRestore the previous global slog logger in test cleanup.
This test mutates process-wide logger state via
slog.SetDefault()but only restoresterm.DefaultTerm. Restore the previousslogdefault as well to prevent cross-test contamination.Proposed fix
func TestComposeGoNoDoubleWarningLog(t *testing.T) { oldTerm := term.DefaultTerm + oldLogger := slog.Default() t.Cleanup(func() { term.DefaultTerm = oldTerm + slog.SetDefault(oldLogger) }) var warnings bytes.Buffer term.DefaultTerm = term.NewTerm(os.Stdin, &warnings, &warnings) slog.SetDefault(logs.NewTermLogger(term.DefaultTerm))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/compose/compose_test.go` around lines 187 - 195, The test sets a new global slog logger with slog.SetDefault(...) but only restores term.DefaultTerm; capture the previous global logger before calling slog.SetDefault (e.g. prevLogger := slog.Default()) and add it to the t.Cleanup so the cleanup restores the original via slog.SetDefault(prevLogger); leave the existing term.DefaultTerm save/restore and logs.NewTermLogger(...) usage intact so the test resets both the terminal and the global slog logger.src/pkg/cli/getServices_test.go (1)
266-274:⚠️ Potential issue | 🟠 MajorRestore the previous slog default logger in cleanup.
term.DefaultTermis restored in the cleanup function, but the global slog default logger set byslog.SetDefault()is not. This causes the modified logger state to leak into subsequent tests, potentially causing test isolation issues and flaky test behavior.Proposed fix
func TestPrintServiceStatesAndEndpointsAndDomainname(t *testing.T) { defaultTerm := term.DefaultTerm + defaultLogger := slog.Default() t.Cleanup(func() { term.DefaultTerm = defaultTerm + slog.SetDefault(defaultLogger) }) var stdout, stderr bytes.Buffer term.DefaultTerm = term.NewTerm(os.Stdin, &stdout, &stderr) slog.SetDefault(logs.NewTermLogger(term.DefaultTerm))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/getServices_test.go` around lines 266 - 274, The test sets a new global slog default via slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) but only restores term.DefaultTerm in cleanup; capture the previous slog default logger (e.g., oldDefault := slog.Default()) before calling slog.SetDefault and restore it in the t.Cleanup closure alongside term.DefaultTerm so the global logger state is reverted after the test; reference term.DefaultTerm, slog.SetDefault, slog.Default(), and logs.NewTermLogger to locate where to capture and restore the original logger.
🧹 Nitpick comments (30)
src/pkg/migrate/platform.go (1)
73-73: Prefer structuredslog.Warnfields instead offmt.Sprintf.Line 73 preformats the log message, which loses structured attributes and adds unnecessary formatting overhead. Use key/value fields directly.
♻️ Proposed change
- slog.Warn(fmt.Sprintf("Invalid source platform selected: %s. Please try again.", selectedOption)) + slog.Warn( + "Invalid source platform selected. Please try again.", + "selectedOption", selectedOption, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/migrate/platform.go` at line 73, The log call currently preformats the message using fmt.Sprintf in the slog.Warn call which loses structured fields; change the call to use structured key/value pairs like slog.Warn("Invalid source platform selected", "selectedOption", selectedOption) (or similar field name) and remove the fmt usage/import if it becomes unused; locate the call to slog.Warn and the variable selectedOption to make this replacement.src/pkg/cli/client/byoc/baseclient.go (1)
112-112: Use structuredslog.Debugwith key/value attributes instead offmt.Sprintln.The current approach works, but
fmt.Sprintlnadds a trailing newline and bypasses slog's structured logging benefits. This pattern appears across the codebase (50+ instances). Prefer:Proposed change
- slog.Debug(fmt.Sprintln("Using default project:", projectNames[0])) + slog.Debug("Using default project", "project", projectNames[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/baseclient.go` at line 112, The current slog.Debug call wraps fmt.Sprintln and emits an unstructured string with a trailing newline; replace that call in baseclient.go so it uses structured attributes instead of fmt.Sprintln: call slog.Debug with a short message like "Using default project" and pass the project name as a key/value attribute (use slog.String or slog.Any) using projectNames[0] as the value so the log is structured and queryable.src/pkg/cli/client/grpc_logger.go (1)
6-7: Use structuredslogfields instead offmt.Sprintlnstring assembly.At Line 42,
fmt.Sprintln(...)collapses fields into one message and appends a newline, which makes output less stable and less queryable. Prefer a constant message + attributes, and drop thefmtimport.Proposed change
import ( "context" "encoding/json" - "fmt" "log/slog" "net/http" @@ - slog.Debug(fmt.Sprintln(g.prefix, requestId, reqType, payload)) + slog.Debug("grpc request", + "prefix", g.prefix, + "request_id", requestId, + "procedure", reqType, + "payload", payload, + )Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/grpc_logger.go` around lines 6 - 7, Replace the fmt.Sprintln usage in the grpc logger with structured slog fields: locate the fmt.Sprintln(...) call (the one currently composing the log message) and change the call to use slog (e.g., slog.Error or slog.Info) with a constant message string and each piece previously concatenated as separate attributes (slog.String, slog.Int, etc.); remove the now-unused "fmt" import from the import block. Ensure you update the call site in grpc_logger.go to pass structured fields rather than a single assembled string so logs remain queryable.src/pkg/github/version.go (1)
38-43: Use structuredslogfields instead of pre-formatting withfmt.Sprintf.Lines 38 and 43 flatten data into strings, weakening structured logs and adding unnecessary formatting overhead.
♻️ Proposed refactor
- slog.Debug(fmt.Sprintf("%v", resp.Header)) + slog.Debug("GitHub API returned non-200 response", "headers", resp.Header) @@ - slog.Debug(fmt.Sprintf("Failed to decode GitHub response: %v", err)) + slog.Debug("Failed to decode GitHub error response", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/github/version.go` around lines 38 - 43, The code is logging structured data by pre-formatting with fmt.Sprintf which loses typed fields; replace those fmt.Sprintf usages with structured slog attributes: log the response headers using slog.Debug with an attribute like slog.Any("resp_header", resp.Header) instead of slog.Debug(fmt.Sprintf("%v", resp.Header)), and log the JSON decode failure using slog.Debug("Failed to decode GitHub response", slog.Any("err", err)) (or slog.String("error", err.Error())) where json.NewDecoder(resp.Body).Decode(&githubError) is called; update the logging around githubError, resp.Header and the Decode error accordingly to use structured slog fields.src/pkg/dockerhub/dockerhub.go (1)
82-82: Prefer structured logging with context-aware method.The current implementation uses
fmt.Sprintfto format the error into the message string, which defeats the purpose of structured logging. Consider usingslog.InfoContextwith structured key-value pairs to preserve the error as a separate field and propagate context.♻️ Suggested structured logging approach
- slog.Info(fmt.Sprintf("Failed to create Docker Hub PAT, fallback to existing docker credentials: %v", err)) + slog.InfoContext(ctx, "Failed to create Docker Hub PAT, fallback to existing docker credentials", "error", err)This approach:
- Captures the error as a structured field for better querying and filtering
- Propagates the context through the logging chain
- Follows Go's slog best practices for structured logging
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dockerhub/dockerhub.go` at line 82, Replace the fmt.Sprintf-based log call that uses slog.Info in the Docker Hub PAT creation fallback with a structured, context-aware slog call: call slog.InfoContext (passing the current ctx) or slog.Info with key/value pairs and include the error as a separate field (e.g., "err": err) and a short message like "Failed to create Docker Hub PAT, falling back to existing docker credentials" so the error remains queryable; locate the call to slog.Info(fmt.Sprintf(...)) in the dockerhub PAT creation/fallback logic and update it to use structured logging with the error as its own key.src/pkg/cli/client/byoc/aws/alb_logs.go (1)
37-37: Use structured slog fields instead offmt.Sprintlnfor better logging semantics.The
fmt.Sprintlncall here produces a formatted string with an unwanted newline and sacrifices structured logging attributes. Prefer native slog key/value pairs:- slog.Debug(fmt.Sprintln("Query ALB logs", bucketPrefix)) + slog.Debug("Query ALB logs", "bucketPrefix", bucketPrefix)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/alb_logs.go` at line 37, Replace the unstructured fmt.Sprintln usage in the slog.Debug call in alb_logs.go with structured slog fields: call slog.Debug with a short message like "Query ALB logs" and attach the bucket prefix as a structured field (e.g., via slog.String or slog.Any) instead of fmt.Sprintln; locate the existing slog.Debug(fmt.Sprintln("Query ALB logs", bucketPrefix)) and change it to use slog.Debug("Query ALB logs", slog.String("bucketPrefix", bucketPrefix)) or equivalent.src/pkg/agent/plugins/compat_oai/generate.go (1)
261-261: Consider using structured logging with slog.The migration is functionally correct, but string concatenation doesn't leverage slog's structured logging capabilities. Consider using key-value pairs for better log parsing and filtering.
✨ Suggested structured logging approach
- slog.Debug("Chat.Completions.NewStreaming: " + string(reqParams)) + slog.Debug("Chat.Completions.NewStreaming", "request", string(reqParams))This enables structured log filtering and parsing while maintaining the same information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/agent/plugins/compat_oai/generate.go` at line 261, Replace the string-concatenated slog.Debug call that logs reqParams with a structured slog.Debug call: locate the slog.Debug invocation around the Chat.Completions.NewStreaming log (the line using slog.Debug(...) and the variable reqParams) and change it to pass the event name as the message and supply reqParams as a key/value pair (e.g., using slog.String or slog.Any) so the request payload is logged as structured data rather than via string concatenation.src/pkg/cli/client/caniuse.go (1)
69-87: Refactor slog calls to use structured attributes instead offmt.Sprintf.The
resolveVersionfunction (lines 69-87) uses eager string formatting withslogmethods, which discards structured fields and adds unnecessary formatting overhead. Convert allslog.Debug(),slog.Info(), andslog.Warn()calls to use message + key/value pairs.♻️ Proposed refactor
- slog.Debug(fmt.Sprintf("Using %s from env: %s", label, fromEnv)) + slog.Debug("Using version from env", "label", label, "version", fromEnv) @@ - slog.Debug(fmt.Sprintf("Using %s: %s", label, fromFabric)) + slog.Debug("Using version", "label", label, "version", fromFabric) @@ - slog.Debug(fmt.Sprintf("Using %s from fabric: %s", label, fromFabric)) - slog.Warn(fmt.Sprintf("Overriding %s: %s", label, forcedReason)) + slog.Debug("Using version from fabric", "label", label, "version", fromFabric) + slog.Warn("Overriding version", "label", label, "reason", forcedReason) @@ - slog.Debug(fmt.Sprintf("Using latest %s: %s", label, fromFabric)) - slog.Info(fmt.Sprintf("Upgrading %s to latest", label)) + slog.Debug("Using latest version", "label", label, "version", fromFabric) + slog.Info("Upgrading to latest", "label", label) @@ - slog.Debug(fmt.Sprintf("Using previous %s: %s", label, previous)) - slog.Warn(fmt.Sprintf("A newer %s is available; using previously deployed version. To upgrade, re-run with --allow-upgrade or set DEFANG_ALLOW_UPGRADE=1", label)) + slog.Debug("Using previous version", "label", label, "version", previous) + slog.Warn("Newer version available; using previously deployed version. To upgrade, re-run with --allow-upgrade or set DEFANG_ALLOW_UPGRADE=1", "label", label)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/caniuse.go` around lines 69 - 87, The slog calls in resolveVersion are using fmt.Sprintf which loses structured fields; replace each slog.Debug/Info/Warn call to use a static message string and key/value pairs referencing label, fromEnv, fromFabric, previous, forcedReason and allowUpgrade (e.g., slog.Debug("using from env", "label", label, "fromEnv", fromEnv)). Update the Debug/Info/Warn sites for the env branch, fabric branch, forcedReason branch, allowUpgrade branch, and previous branch so they pass structured attributes instead of formatted strings.src/pkg/cli/client/playground.go (1)
187-187: Use structured slog fields instead offmt.Sprintlnfor debug logging.Line 187 currently formats eagerly with
fmt.Sprintln, which appends a newline and loses structured context. Useslog.Debugwith key-value pairs for proper structured logging.♻️ Proposed fix
-import ( +import ( "context" "errors" - "fmt" "io" "iter" "log/slog" "os" @@ - slog.Debug(fmt.Sprintln("Using default Playground project: ", resp.Project)) + slog.Debug("Using default Playground project", "project", resp.Project)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/playground.go` at line 187, Replace the eager fmt.Sprintln call used in slog.Debug so the log is emitted with structured key/value fields; locate the slog.Debug call that currently wraps fmt.Sprintln and change it to call slog.Debug with a descriptive message and the project as a structured field (use resp.Project as the value) so the log retains structured context instead of a formatted string.src/pkg/cli/client/byoc/state/parse.go (1)
89-94: Use structured slog arguments instead of fmt.Sprintln for better log traceability.
slog.Debug(fmt.Sprintln(...))converts arguments to an unstructured string and loses the ability to query/filter logs by structured fields. Use the message parameter with key-value attributes directly.♻️ Suggested patch
- slog.Debug(fmt.Sprintln("Skipping Pulumi state with version", state.Version)) + slog.Debug("Skipping Pulumi state with version", "version", state.Version)- slog.Debug(fmt.Sprintln("Skipping pending operation with malformed URN:", op.Resource.Urn)) + slog.Debug("Skipping pending operation with malformed URN", "urn", op.Resource.Urn)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/state/parse.go` around lines 89 - 94, The logs in the pending-operations branch use fmt.Sprintln inside slog.Debug (e.g., the calls around state.Checkpoint.Latest.PendingOperations and the earlier "Skipping Pulumi state with version" line), which produces unstructured strings; replace those with structured slog calls by passing a short message string and key/value attributes (e.g., "skipping pulumi state" with slog.Int("version", state.Version) and "skipping pending operation" with slog.String("urn", op.Resource.Urn) or other relevant fields) so logs are queryable; locate the slog.Debug(fmt.Sprintln(...)) occurrences in parse.go and change them to slog.Debug("message", slog.String("urn", ...), ...) accordingly.src/pkg/cli/client/projectName.go (1)
17-20: Use structuredslog.Debugfields instead offmt.Sprintlnmessage assembly.Line 17 currently builds the message via
fmt.Sprintln, which adds a trailing newline and bypasses structured fields. Prefer native slog key/value attributes.Proposed diff
- slog.Debug(fmt.Sprintln("Failed to load local project:", err)) + slog.Debug("Failed to load local project", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/projectName.go` around lines 17 - 20, Replace the fmt.Sprintln-based log call with a structured slog.Debug call so the error is emitted as a key/value attribute; locate the slog.Debug invocation that currently passes fmt.Sprintln("Failed to load local project:", err) (near where loadErr is set) and change it to use structured fields (e.g., message "Failed to load local project" with an "err" or "error" field carrying the error via slog.Any/slog.String as appropriate).src/pkg/cli/cert.go (1)
73-73: Replace eagerfmt.Sprintfcalls insideslogwith structured key/value arguments.The current pattern works, but it drops structured attributes and causes eager formatting on every call regardless of log level. Refactor to use native slog key/value pairs for better performance and structured logging.
All occurrences in this file use this pattern: 73, 81, 99, 107, 136, 138, 142, 144, 148, 151, 157, 159, 164, 180, 209, 238, 242, 244, 249, 255, 268, 299, 306, 310.
Refactor example
- slog.Debug(fmt.Sprintf("Redirecting from %v to %v", via[len(via)-1].URL, req.URL)) + slog.Debug("Redirecting", "from", via[len(via)-1].URL, "to", req.URL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/cert.go` at line 73, Replace eager fmt.Sprintf logging calls in cert.go with structured slog key/value arguments: find occurrences (e.g., the redirect log where slog.Debug(fmt.Sprintf("Redirecting from %v to %v", via[len(via)-1].URL, req.URL)) is used) and change them to use slog.Debug with a message and key/value pairs (for example, slog.Debug("redirect", "from", via[len(via)-1].URL, "to", req.URL) or using slog.String/Any helpers if available). Apply the same refactor for all similar calls in this file (the other slog.* calls listed in the review) so they stop performing eager fmt.Sprintf and instead emit structured attributes.src/pkg/cli/waitForCdTaskExit.go (1)
22-22: Consider removing the commented debug line to reduce noise.Non-functional, but dropping dead/commented logging keeps this polling loop cleaner.
🧹 Optional cleanup
- // slog.Debug(fmt.Sprintf("Polled CD task status: done=%v, err=%v", done, err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/waitForCdTaskExit.go` at line 22, The commented-out debug log inside the polling loop in waitForCdTaskExit.go (the line containing // slog.Debug(fmt.Sprintf("Polled CD task status: done=%v, err=%v", done, err))) is dead code and should be removed to clean up noise; locate the polling loop in the waitForCdTaskExit function and delete that commented slog.Debug(...) line so the loop contains only active logic and meaningful logs.src/pkg/cli/connect.go (1)
18-18: Prefer native structured slog fields overfmt.Sprintf/fmt.Sprintln.This keeps logs queryable and avoids
fmt.Sprintln's trailing newline behavior. Convert preformatted strings to slog's key-value attribute pattern:♻️ Suggested refactor
- slog.Debug(fmt.Sprintf("Using tenant %q for cluster %q", requestedTenant, host)) + slog.Debug("Using tenant for cluster", "tenant", requestedTenant, "cluster", host) - slog.Debug(fmt.Sprintln("Unable to validate tenant with server:", err)) + slog.Debug("Unable to validate tenant with server", "error", err) - slog.Debug(fmt.Sprintf("Creating %s provider", providerID)) + slog.Debug("Creating provider", "provider", providerID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/connect.go` at line 18, Replace the preformatted debug call that uses fmt.Sprintf in connect.go with structured slog fields: instead of slog.Debug(fmt.Sprintf("Using tenant %q for cluster %q", requestedTenant, host)) call slog.Debug with a short message and key/value attributes (use slog.String or slog.Any for requestedTenant and host) so logs are structured and queryable; also remove the fmt import if it becomes unused.src/pkg/agent/tools/listConfig.go (1)
23-24: Align debug labels with the actual invoked methods.The log strings currently reference
cli.Connect/cli.ConfigList, but the calls areGetClientWithRetry/cli.ListConfig. Updating labels will make traces less confusing.✏️ Suggested text-only fix
- slog.Debug("Function invoked: cli.Connect") + slog.Debug("Function invoked: GetClientWithRetry") @@ - slog.Debug("Function invoked: cli.ConfigList") + slog.Debug("Function invoked: cli.ListConfig")Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/agent/tools/listConfig.go` around lines 23 - 24, The debug log labels are mismatched: they reference cli.Connect / cli.ConfigList while the code actually calls GetClientWithRetry and cli.ListConfig; update the slog.Debug messages near the GetClientWithRetry invocation and the cli.ListConfig invocation to accurately name GetClientWithRetry and cli.ListConfig respectively (look for slog.Debug calls around GetClientWithRetry(ctx, cli, sc) and cli.ListConfig(ctx, client, sc) and change the text to match those function names).src/pkg/cli/client/byoc/aws/subscribe.go (1)
58-58: Use structuredslogfields instead of preformatted strings.At line 58,
fmt.Sprintfdiscards structured metadata and allocates unnecessary strings. Use native key/value fields instead, which also allows removing the unusedfmtimport.♻️ Proposed refactor
-import ( - "fmt" - "iter" - "log/slog" - "slices" - "strings" +import ( + "iter" + "log/slog" + "slices" + "strings" @@ - slog.Debug(fmt.Sprintf("error parsing ECS event: %v", err)) + slog.Debug("error parsing ECS event", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/subscribe.go` at line 58, Replace the preformatted string logging call slog.Debug(fmt.Sprintf("error parsing ECS event: %v", err)) with a structured slog call such as slog.Debug("error parsing ECS event", slog.Any("err", err)) (or slog.Debug("error parsing ECS event", "err", err) depending on your slog helpers) so the error is logged as a field; also remove the now-unused fmt import. Locate the slog.Debug call in subscribe.go (the line that currently uses fmt.Sprintf) and update it accordingly.src/pkg/cli/composeDown.go (1)
17-38: Prefer structured slog fields over string formatting/concatenation.This improves consistency and structured log output.
♻️ Suggested update
- slog.Debug(fmt.Sprintf("Destroying project %q", projectName)) + slog.Debug("Destroying project", "project", projectName) ... - slog.Info("Deactivating project " + projectName) + slog.Info("Deactivating project", "project", projectName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/composeDown.go` around lines 17 - 38, Replace unstructured slog.Debug(fmt.Sprintf("Destroying project %q", projectName)) and slog.Info("Deactivating project "+projectName) with structured slog calls: use slog.Debug/Info with key/value fields (e.g., slog.String("project", projectName) and any other context like "action":"destroy" or "action":"deactivate") so logs are emitted as structured fields; update the logging sites in InteractiveComposeDown and the earlier destroy path (the call that currently uses fmt.Sprintf) to remove string concatenation/formatting and pass structured attributes instead.src/pkg/mcp/setup.go (1)
346-359: Prefer structuredslogfields instead offmt.Sprintfand embedded newlines.Current calls serialize text eagerly and include
\n, which weakens structured logging and can cause formatting inconsistencies in handlers.♻️ Suggested update
- slog.Info(fmt.Sprintf("Updating %q\n", configPath)) + slog.Info("Updating MCP client config", "path", configPath) ... - slog.Info(fmt.Sprintf("Ensure %s is upgraded to the latest version and restarted for MCP settings to take effect.\n", client)) + slog.Info("Ensure client is upgraded to the latest version and restarted for MCP settings to take effect", "client", client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/mcp/setup.go` around lines 346 - 359, The log calls use fmt.Sprintf with embedded "\n" which defeats structured logging; replace the two slog.Info(fmt.Sprintf(...)) calls with structured slog.Info calls that pass message strings (no trailing newlines) and key/value fields for configPath and client (e.g. slog.Info("Updating config", "configPath", configPath) and slog.Info("MCP upgrade required", "client", client, "note", "ensure upgraded and restarted")); update the call site around configureDefangMCPServer and keep error handling intact, referencing the existing symbols configPath, client, configureDefangMCPServer, and slog.Info.src/pkg/agent/generator.go (1)
77-77: Use structured error fields withslog.Debug.This keeps logs queryable and avoids string formatting at call sites.
♻️ Suggested update
- slog.Debug(fmt.Sprintf("error: %v", err)) + slog.Debug("generator turn failed; retrying", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/agent/generator.go` at line 77, Replace the unstructured fmt.Sprintf log with a structured slog call: instead of slog.Debug(fmt.Sprintf("error: %v", err)) use slog.Debug with key/value fields (e.g. slog.Debug("operation error", "err", err) or slog.Debug("operation error", slog.Any("err", err))) so the error is logged as a structured field; locate the site where err is logged in generator.go and update that slog.Debug invocation accordingly.src/pkg/mcp/mcp_server.go (1)
33-39: Prefer structured slog calls for MCP tool logs.This will improve queryability and avoid newline artifacts from
fmt.Sprintln.Proposed refactor
- slog.Debug("MCP Tool Called: " + name + " with params: " + fmt.Sprintf("%+v", request.Params)) + slog.Debug("MCP Tool Called", "tool", name, "params", request.Params) @@ - slog.Error(fmt.Sprintln("MCP Tool Failed: "+name, "error", err)) + slog.Error("MCP Tool Failed", "tool", name, "error", err) } else { - slog.Debug("MCP Tool Succeeded: " + name) + slog.Debug("MCP Tool Succeeded", "tool", name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/mcp/mcp_server.go` around lines 33 - 39, Replace the string-concatenation and fmt.Sprintln logging with structured slog calls: change slog.Debug("MCP Tool Called: "+name+"...") to slog.Debug("MCP Tool Called", "tool", name, "params", request.Params, "client", t.client, "cluster", t.fabricAddr, "provider", t.providerId) and change the error branch to slog.Error("MCP Tool Failed", "tool", name, "error", err) (use the same structured fields around the handler invocation and after resp is returned); update the success log to slog.Debug("MCP Tool Succeeded", "tool", name) so all logs use structured keys and avoid fmt.Sprintln/newline artifacts while keeping references to handler, name, request.Params, t.client, t.fabricAddr, and t.providerId.src/pkg/cli/configList.go (1)
18-18: Prefer structuredslogfields over preformatted debug strings.This currently does eager formatting and drops structured fields. Consider:
Proposed refactor
import ( "context" - "fmt" "log/slog" @@ - slog.Debug(fmt.Sprintf("Listing config in project %q", projectName)) + slog.Debug("Listing config in project", "project", projectName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/configList.go` at line 18, Replace the eager fmt.Sprintf call passed to slog.Debug with a structured slog call so fields are preserved and formatting is deferred; in the config listing code (the call currently using slog.Debug(fmt.Sprintf("Listing config in project %q", projectName))) change it to something like slog.Debug("Listing config in project", slog.String("project", projectName)) and remove the now-unused fmt import if applicable.src/cmd/cli/command/whoami.go (1)
30-30: Keep warnings structured instead of usingfmt.Sprintln/fmt.Sprintf.This improves consistency and avoids accidental newline artifacts in warning output.
♻️ Proposed fix
- slog.Warn(fmt.Sprintf("Provider account information not available: %v", err)) + slog.Warn("Provider account information not available", "error", err) - slog.Warn(fmt.Sprintln("Workspace information unavailable:", err)) + slog.Warn("Workspace information unavailable", "error", err)Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/whoami.go` at line 30, Replace the use of fmt.Sprintf inside slog.Warn in the whoami command with structured logging: in the function handling provider account lookup (the slog.Warn calls around the references at the current occurrences near the fmt.Sprintf usage), remove fmt.Sprintf and pass a static message plus a key/value pair for the error (e.g., "error" or "err" paired with the err value) so calls become structured slog.Warn("Provider account information not available", "error", err); update both occurrences (the one at the earlier fmt.Sprintf and the similar one around line 43) to follow this pattern.src/pkg/setup/setup.go (1)
247-247: Avoidfmt.Sprintlninsideslog.Info.Use a fixed message plus attrs to avoid trailing newline and keep logs structured.
♻️ Proposed fix
- slog.Info(fmt.Sprintln("Compose file written to", composeFilePath)) + slog.Info("Compose file written", "path", composeFilePath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/setup/setup.go` at line 247, The log call uses fmt.Sprintln inside slog.Info which produces an extra newline and bypasses structured logging; update the call to use a fixed message and attributes instead (replace slog.Info(fmt.Sprintln("Compose file written to", composeFilePath)) with a structured call using slog.Info("Compose file written to", slog.String("compose_file", composeFilePath)) so the message is constant and the path is an attribute), referencing the slog.Info call and the composeFilePath variable in setup.go.src/pkg/agent/tools/deploy.go (1)
60-60: Use structuredslogfields for deploy context.Prefer attrs instead of preformatted text for project metadata in debug logs.
♻️ Proposed fix
- slog.Debug(fmt.Sprintf("Deploying services for project %s...", project.Name)) + slog.Debug("Deploying services...", "project", project.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/agent/tools/deploy.go` at line 60, Replace the preformatted message at the slog.Debug call in src/pkg/agent/tools/deploy.go with structured slog attributes: instead of slog.Debug(fmt.Sprintf("Deploying services for project %s...", project.Name)) call slog.Debug("Deploying services", slog.String("project", project.Name)) (and add other relevant fields like project.ID or project.Namespace via slog.String/Int as available) so the deploy context is emitted as structured fields rather than a formatted string.src/pkg/agent/tools/logs.go (1)
70-70: Prefer structuredslogfields over preformatted strings.Using
fmt.Sprintlninsideslogdrops structured key/value data and adds trailing newline noise. Pass attrs directly.♻️ Proposed fix
- slog.Debug(fmt.Sprintln("Project name loaded:", projectName)) + slog.Debug("Project name loaded", "project_name", projectName) - slog.Error(fmt.Sprintln("Failed to fetch logs", "error", err)) + slog.Error("Failed to fetch logs", "error", err)Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/agent/tools/logs.go` at line 70, Replace the fmt.Sprintln usage inside slog.Debug with structured slog attributes: locate the slog.Debug calls (e.g., the call using fmt.Sprintln("Project name loaded:", projectName) and the similar occurrence at the other site) and change them to pass a short message plus key/value attrs (use slog.String or slog.Any for the "project" key and projectName value) so logs remain structured and avoid trailing newlines; update all occurrences of slog.Debug(fmt.Sprintln(...)) in this file accordingly.src/pkg/stacks/manager.go (1)
111-111: Avoidfmt.Sprintf-built slog messages (and remove embedded\n).These calls should keep message text stable and put variable values into slog attrs; the newline in the warning text is also unnecessary.
♻️ Proposed fix
- slog.Warn(fmt.Sprintf("Skipping invalid remote stack %s: %v\n", name, err)) + slog.Warn("Skipping invalid remote stack", "stack", name, "error", err) - slog.Info(fmt.Sprintf("stack file not found, attempting to import from previous deployments: %v", err)) + slog.Info("stack file not found, attempting import from previous deployments", "error", err) - slog.Info(fmt.Sprintf("Stack %q loaded and saved to %q. Add this file to source control.", name, stackFilename)) + slog.Info("Stack loaded and saved. Add this file to source control.", "stack", name, "file", stackFilename) - slog.Warn(fmt.Sprintf("Using default stack %q for project %q, but the stack specifies COMPOSE_FILE=%q", res.Stack.Name, sm.projectName, cf)) + slog.Warn("Default stack specifies COMPOSE_FILE", "stack", res.Stack.Name, "project", sm.projectName, "compose_file", cf)Also applies to: 153-153, 286-286, 327-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/stacks/manager.go` at line 111, Replace the fmt.Sprintf-based slog warnings that embed a newline with structured slog calls: remove the fmt.Sprintf and "\n", pass a fixed message string like "Skipping invalid remote stack" to slog.Warn and supply the dynamic values as slog attributes (e.g., stack/name and err) instead of interpolating them; update all occurrences where slog.Warn(fmt.Sprintf(...)) uses variables name and err (the occurrences you saw around the existing call and the similar calls later in the file) to follow this pattern so logs are stable and structured.src/pkg/auth/auth.go (1)
76-76: Prefer attr-basedsloglogging in auth flow.Current calls are functional, but formatting values into strings reduces structured observability.
♻️ Proposed fix
- slog.Error(fmt.Sprintf("failed to poll for auth code: %v", err)) + slog.Error("failed to poll for auth code", "error", err) - slog.Error(fmt.Sprintf("failed to exchange code for token: %v", err)) + slog.Error("failed to exchange code for token", "error", err) - slog.Debug(fmt.Sprintf("received server error: %s, retrying in %v...", unexpectedError.Status, retryDelay)) + slog.Debug("received server error, retrying", "status", unexpectedError.Status, "retry_delay", retryDelay) - slog.Debug(fmt.Sprintf("Generating access token with scopes %v", scopes)) + slog.Debug("Generating access token", "scopes", scopes)Also applies to: 82-82, 120-120, 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/auth/auth.go` at line 76, Replace string-formatted slog calls that embed errors with attr-based structured logging: locate the slog.Error usages that currently call fmt.Sprintf (e.g., slog.Error(fmt.Sprintf("failed to poll for auth code: %v", err))) and change them to pass a static message plus an error attribute (for example slog.Error("failed to poll for auth code", "err", err) or slog.Error("failed to poll for auth code", slog.Any("err", err))). Do this for all similar occurrences in auth.go (the messages like "failed to poll for auth code" and other fmt.Sprintf-based error logs at the indicated locations) so logs are emitted with structured attributes rather than formatted strings.src/pkg/clouds/aws/login.go (1)
75-92: Standardize AWS login logs to structured fields.The migration is good, but these calls still mostly preformat strings; also remove embedded newline in debug text.
♻️ Example fix pattern
- slog.Debug(fmt.Sprintf("checking default AWS credentials for region %s...", a.Region)) + slog.Debug("checking default AWS credentials", "region", a.Region) - slog.Warn(fmt.Sprintf("failed to get AWS_PROFILE role ARN: %v", err)) + slog.Warn("failed to get AWS_PROFILE role ARN", "error", err) - slog.Debug(fmt.Sprintf("token %q is valid and can assume AWS_PROFILE role %q\n", name, roleArn)) + slog.Debug("token is valid and can assume AWS_PROFILE role", "token", name, "role_arn", roleArn) - slog.Warn(fmt.Sprintf("failed to parse redirect URL %q, assume port 8080: %v", redirectURL, err)) + slog.Warn("failed to parse redirect URL, using default port", "redirect_url", redirectURL, "port", "8080", "error", err)Also applies to: 130-137, 260-297, 342-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 75 - 92, Replace preformatted string logs and the embedded newline with structured slog fields: change slog.Debug(fmt.Sprintf(...)) / slog.Warn(fmt.Sprintf(...)) to calls like slog.Debug("AWS OAuth access token expired, refreshing", slog.String("token_store_key", p.storeKey)) or slog.Warn("failed to persist refreshed AWS OAuth token", slog.Any("err", err), slog.String("token_store_key", p.storeKey)). Do this in the refresh block around refreshToken(ctx, p.cached) and the persist block using p.tokenStore and p.storeKey (and apply same pattern to the other occurrences you noted). Ensure messages are plain strings (no fmt.Sprintf) and attach error/details via slog.* field helpers; also remove any embedded newline characters from the debug message.src/pkg/clouds/gcp/iam.go (1)
199-199: Minor: Trailing newlines are redundant.The
sloghandler routes throughWriteWarn/WriteDebugwhich callensureNewline(), so the explicit\nin these format strings is redundant and will result in double newlines.♻️ Suggested cleanup
- slog.Info(fmt.Sprintf("Failed to set IAM policy, will retry in %v: %v\n", retryInterval, err)) + slog.Info(fmt.Sprintf("Failed to set IAM policy, will retry in %v: %v", retryInterval, err))- slog.Info(fmt.Sprintf("Failed to set IAM policy for service account %s, will retry in %v: %v\n", serviceAccount, retryInterval, err)) + slog.Info(fmt.Sprintf("Failed to set IAM policy for service account %s, will retry in %v: %v", serviceAccount, retryInterval, err))- slog.Debug(fmt.Sprintf("Failed to set IAM policy for resource %s, will retry in %v: %v\n", resource, retryInterval, err)) + slog.Debug(fmt.Sprintf("Failed to set IAM policy for resource %s, will retry in %v: %v", resource, retryInterval, err))Also applies to: 279-279, 356-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/iam.go` at line 199, Remove the redundant trailing "\n" from the slog log format strings (e.g., the fmt.Sprintf passed to slog.Info in the IAM policy retry path), since the slog handler already ensures newlines; update the other similar occurrences noted (the fmt.Sprintf calls at the other slog invocations referenced around lines 279 and 356) to use the same message text without the trailing newline so logs don't produce double blank lines.src/cmd/cli/command/session.go (1)
170-170: Minor:fmt.Sprintlnadds trailing newline unnecessarily.Using
fmt.Sprintlnwithslog.Errorresults in a trailing newline being passed to the handler, which then adds another viaensureNewline(). Consider usingfmt.Sprintorfmt.Sprintfinstead.♻️ Suggested fix
- slog.Error(fmt.Sprintln("Cannot load project:", loadErr)) + slog.Error(fmt.Sprintf("Cannot load project: %v", loadErr))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/session.go` at line 170, Replace the fmt.Sprintln call passed to slog.Error so it doesn't include a trailing newline; locate the slog.Error invocation that currently reads slog.Error(fmt.Sprintln("Cannot load project:", loadErr)) and change it to use fmt.Sprint or fmt.Sprintf (e.g., fmt.Sprint("Cannot load project: ", loadErr)) to avoid the extra newline added by fmt.Sprintln and the handler's ensureNewline().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b55f0740-e15a-4a11-a0b7-d1a34566a2fb
📒 Files selected for processing (116)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/compose.gosrc/cmd/cli/command/compose_test.gosrc/cmd/cli/command/config.gosrc/cmd/cli/command/estimate.gosrc/cmd/cli/command/generate.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/login.gosrc/cmd/cli/command/logout.gosrc/cmd/cli/command/mcp.gosrc/cmd/cli/command/session.gosrc/cmd/cli/command/stack.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/whoami.gosrc/cmd/cli/command/workspace.gosrc/pkg/agent/common/common.gosrc/pkg/agent/generator.gosrc/pkg/agent/plugins/compat_oai/generate.gosrc/pkg/agent/toolmanager.gosrc/pkg/agent/tools/deploy.gosrc/pkg/agent/tools/destroy.gosrc/pkg/agent/tools/estimate.gosrc/pkg/agent/tools/listConfig.gosrc/pkg/agent/tools/logs.gosrc/pkg/agent/tools/provider.gosrc/pkg/agent/tools/removeConfig.gosrc/pkg/agent/tools/services.gosrc/pkg/agent/tools/setConfig.gosrc/pkg/auth/auth.gosrc/pkg/cli/cd.gosrc/pkg/cli/cert.gosrc/pkg/cli/client/byoc/aws/alb_logs.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/domain.gosrc/pkg/cli/client/byoc/aws/list.gosrc/pkg/cli/client/byoc/aws/stream.gosrc/pkg/cli/client/byoc/aws/subscribe.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/client/byoc/common.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/state/parse.gosrc/pkg/cli/client/caniuse.gosrc/pkg/cli/client/cluster.gosrc/pkg/cli/client/grpc_logger.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/pretty_error.gosrc/pkg/cli/client/projectName.gosrc/pkg/cli/common.gosrc/pkg/cli/compose/baseimage.gosrc/pkg/cli/compose/compose_test.gosrc/pkg/cli/compose/context.gosrc/pkg/cli/compose/dockerfile_validator.gosrc/pkg/cli/compose/fixup.gosrc/pkg/cli/compose/loader.gosrc/pkg/cli/compose/serviceNameReplacer.gosrc/pkg/cli/compose/serviceNameReplacer_test.gosrc/pkg/cli/compose/validation.gosrc/pkg/cli/compose/validation_test.gosrc/pkg/cli/composeDown.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/configDelete.gosrc/pkg/cli/configList.gosrc/pkg/cli/configList_test.gosrc/pkg/cli/configResolution.gosrc/pkg/cli/configResolution_test.gosrc/pkg/cli/configSet.gosrc/pkg/cli/connect.gosrc/pkg/cli/deploymentsList.gosrc/pkg/cli/deploymentsList_test.gosrc/pkg/cli/estimate.gosrc/pkg/cli/generate.gosrc/pkg/cli/getServices.gosrc/pkg/cli/getServices_test.gosrc/pkg/cli/install_cd.gosrc/pkg/cli/logout.gosrc/pkg/cli/new.gosrc/pkg/cli/subscribe.gosrc/pkg/cli/tail.gosrc/pkg/cli/tailAndMonitor.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/teardown_cd.gosrc/pkg/cli/token.gosrc/pkg/cli/upgrade.gosrc/pkg/cli/waitForCdTaskExit.gosrc/pkg/cli/whoami.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/login.gosrc/pkg/clouds/do/appPlatform/setup.gosrc/pkg/clouds/gcp/api.gosrc/pkg/clouds/gcp/iam.gosrc/pkg/clouds/gcp/logging.gosrc/pkg/clouds/gcp/login.gosrc/pkg/clouds/gcp/storage.gosrc/pkg/debug/debug_test.gosrc/pkg/dockerhub/dockerhub.gosrc/pkg/github/version.gosrc/pkg/login/agree_tos.gosrc/pkg/login/login.gosrc/pkg/logs/slog.gosrc/pkg/mcp/mcp_server.gosrc/pkg/mcp/resources/resources.gosrc/pkg/mcp/setup.gosrc/pkg/mcp/utils.gosrc/pkg/migrate/heroku.gosrc/pkg/migrate/migrate.gosrc/pkg/migrate/platform.gosrc/pkg/session/session.gosrc/pkg/setup/setup.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/stacks.gosrc/pkg/term/colorizer.gosrc/pkg/tokenstore/store.gosrc/pkg/track/track.go
- Fix %s→%v format verb for []string in agent/common debug log - Move "project name loaded" log after error check in agent/tools/services - Fix fmt.Sprintln→fmt.Sprintf in cd.go, config.go, teardown_cd.go (removes trailing newlines) - Include domain and err in cert generation failure log - Fix potential panic truncating multi-byte strings in logs/slog.go - Remove raw Heroku CLI output from debug log; return explicit error for empty token - Add isWithinBase() using filepath.Rel to fix sibling-directory traversal in tokenstore - Update slog.SetDefault after term.DefaultTerm reassignment in mcp.go - Add slog.Default() save/restore cleanup in test subtests to prevent global state leakage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pkg/cli/deploymentsList_test.go (1)
67-69: Optional: extract repeated slog setup into a tiny test helper.This block appears 4 times; a helper would reduce duplication and make future logger setup changes safer.
♻️ Example refactor
+func setTestSlogToTerm(t *testing.T) { + prevLogger := slog.Default() + t.Cleanup(func() { slog.SetDefault(prevLogger) }) + slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) +} ... - prevLogger := slog.Default() - t.Cleanup(func() { slog.SetDefault(prevLogger) }) - slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)) + setTestSlogToTerm(t)Also applies to: 89-91, 144-146, 183-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/deploymentsList_test.go` around lines 67 - 69, Extract the repeated slog setup (saving prevLogger, t.Cleanup to restore, and slog.SetDefault(logs.NewTermLogger(term.DefaultTerm))) into a small test helper function like setupTestLogger(t *testing.T) or withTestLogger(t *testing.T) and call it from each test instead of duplicating the three lines; locate the block in deploymentsList_test.go (currently repeated at the snippets around lines 67-69, 89-91, 144-146, 183-185) and replace the three-line sequence with a single call to the new helper which performs prevLogger := slog.Default(); t.Cleanup(func(){ slog.SetDefault(prevLogger) }); slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)).src/pkg/migrate/heroku.go (1)
40-80: Prefer nativeslogattrs instead offmt.Sprintfmessages.These calls still build the final log string eagerly, and the collection dumps also carry trailing
\n. Using message + attrs keeps the output consistent and preserves structured fields.Suggested cleanup
- slog.Debug(fmt.Sprintf("Dynos for the selected application: %+v\n", dynos)) + slog.Debug("Dynos for the selected application", "dynos", dynos) - slog.Debug(fmt.Sprintf("Failed to run `heroku login`: %v", err)) + slog.Debug("Failed to run `heroku login`", "error", err) - slog.Debug(fmt.Sprintf("Failed to parse Heroku CLI output: %v", err)) + slog.Debug("Failed to parse Heroku CLI output", "error", err)Also applies to: 330-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/migrate/heroku.go` around lines 40 - 80, The logs currently build strings with fmt.Sprintf (e.g., slog.Debug(fmt.Sprintf("Dynos for the selected application: %+v\n", dynos))) which eagerly formats values and includes trailing newlines; replace these with structured slog calls that pass a short message and attrs (e.g., slog.Debug("Dynos for the selected application", slog.Any("dynos", dynos))) for all occurrences in this snippet (Dynos, dynoSizes, releaseTasks, addons, applicationInfo.PGInfo) and the other similar blocks referenced (lines ~330-373), ensuring you remove the "\n" and use appropriate slog helpers (slog.Any, slog.String, slog.Int, etc.) so fields are emitted as structured attributes rather than concatenated strings.src/pkg/tokenstore/store.go (1)
35-35: Use structuredslogattributes instead of preformatted strings.At Lines 35 and 53,
fmt.Sprintln(...)adds a trailing newline and drops structured fields. Same for Line 90 (Sprintf) and Line 120 (string concatenation). Prefer message + attrs.Proposed refactor
- slog.Debug(fmt.Sprintln("Saving access token to", tokenFile)) + slog.Debug("Saving access token", "path", tokenFile) ... - slog.Debug(fmt.Sprintln("Reading access token from file", tokenFile)) + slog.Debug("Reading access token", "path", tokenFile) ... - slog.Warn(fmt.Sprintf("Invalid token prefix %q: resolved directory %q is outside of token store base directory %q", prefix, dir, baseDir)) + slog.Warn("Invalid token prefix resolved outside token store base directory", + "prefix", prefix, + "resolved_dir", dir, + "base_dir", baseDir, + ) ... - slog.Debug("Removed token file: " + tokenFile) + slog.Debug("Removed token file", "path", tokenFile)Also applies to: 53-53, 90-90, 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/tokenstore/store.go` at line 35, The logs in store.go use fmt.Sprintln / Sprintf / string concatenation inside slog.Debug which loses structured fields and adds newlines; replace each call (the slog.Debug invocations around the token save/load functions and any other occurrences at the noted sites) to use a message plus structured attributes, e.g. slog.Debug("Saving access token", slog.String("tokenFile", tokenFile)) or slog.Debug("Loaded access token", slog.String("tokenFile", tokenFile), slog.Int("bytes", len(data))) — avoid fmt.* and concatenation and prefer slog.String/slog.Int/slog.Any attributes for the slog.Debug calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/tokenstore/store.go`:
- Around line 61-67: isWithinBase currently treats rel == "." as within base
which allows operations on the store root; change its signature to
isWithinBase(baseDir, target string, allowEqual bool) and adjust the return so
rel == "." is considered inside only when allowEqual is true (i.e., return
allowEqual || rel != "." and keep the existing checks for ".." and absolute
paths). Update all call sites: have List call isWithinBase(..., true) to permit
root listing, and have getTokenFile (and any delete/token-modifying paths) call
isWithinBase(..., false) so operations that resolve to the store root are
rejected. Ensure error handling and tests reflect the new behavior.
---
Nitpick comments:
In `@src/pkg/cli/deploymentsList_test.go`:
- Around line 67-69: Extract the repeated slog setup (saving prevLogger,
t.Cleanup to restore, and slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)))
into a small test helper function like setupTestLogger(t *testing.T) or
withTestLogger(t *testing.T) and call it from each test instead of duplicating
the three lines; locate the block in deploymentsList_test.go (currently repeated
at the snippets around lines 67-69, 89-91, 144-146, 183-185) and replace the
three-line sequence with a single call to the new helper which performs
prevLogger := slog.Default(); t.Cleanup(func(){ slog.SetDefault(prevLogger) });
slog.SetDefault(logs.NewTermLogger(term.DefaultTerm)).
In `@src/pkg/migrate/heroku.go`:
- Around line 40-80: The logs currently build strings with fmt.Sprintf (e.g.,
slog.Debug(fmt.Sprintf("Dynos for the selected application: %+v\n", dynos)))
which eagerly formats values and includes trailing newlines; replace these with
structured slog calls that pass a short message and attrs (e.g.,
slog.Debug("Dynos for the selected application", slog.Any("dynos", dynos))) for
all occurrences in this snippet (Dynos, dynoSizes, releaseTasks, addons,
applicationInfo.PGInfo) and the other similar blocks referenced (lines
~330-373), ensuring you remove the "\n" and use appropriate slog helpers
(slog.Any, slog.String, slog.Int, etc.) so fields are emitted as structured
attributes rather than concatenated strings.
In `@src/pkg/tokenstore/store.go`:
- Line 35: The logs in store.go use fmt.Sprintln / Sprintf / string
concatenation inside slog.Debug which loses structured fields and adds newlines;
replace each call (the slog.Debug invocations around the token save/load
functions and any other occurrences at the noted sites) to use a message plus
structured attributes, e.g. slog.Debug("Saving access token",
slog.String("tokenFile", tokenFile)) or slog.Debug("Loaded access token",
slog.String("tokenFile", tokenFile), slog.Int("bytes", len(data))) — avoid fmt.*
and concatenation and prefer slog.String/slog.Int/slog.Any attributes for the
slog.Debug calls.
🪄 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: 0f632bbf-b7c1-45fe-988d-5a5f642126cc
📒 Files selected for processing (15)
src/cmd/cli/command/config.gosrc/cmd/cli/command/mcp.gosrc/pkg/agent/common/common.gosrc/pkg/agent/tools/services.gosrc/pkg/cli/cd.gosrc/pkg/cli/cert.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/configList_test.gosrc/pkg/cli/configResolution_test.gosrc/pkg/cli/deploymentsList_test.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/teardown_cd.gosrc/pkg/logs/slog.gosrc/pkg/migrate/heroku.gosrc/pkg/tokenstore/store.go
✅ Files skipped from review due to trivial changes (5)
- src/pkg/cli/teardown_cd.go
- src/pkg/agent/common/common.go
- src/pkg/agent/tools/services.go
- src/cmd/cli/command/mcp.go
- src/pkg/logs/slog.go
🚧 Files skipped from review as they are similar to previous changes (6)
- src/cmd/cli/command/config.go
- src/pkg/cli/configResolution_test.go
- src/pkg/cli/cd.go
- src/pkg/cli/tail_test.go
- src/pkg/cli/client/byoc/aws/byoc_test.go
- src/pkg/cli/cert.go
Comment on lines
+61
to
+67
| func isWithinBase(baseDir, target string) bool { | ||
| rel, err := filepath.Rel(baseDir, target) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) && !filepath.IsAbs(rel)) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/pkg/tokenstore/store.go | sed -n '55,150p'Repository: DefangLabs/defang
Length of output: 3344
🏁 Script executed:
rg 'isWithinBase' src/pkg/tokenstore/store.go -nRepository: DefangLabs/defang
Length of output: 199
🏁 Script executed:
find src/pkg/tokenstore -name "*test*" -type fRepository: DefangLabs/defang
Length of output: 93
🏁 Script executed:
cat -n src/pkg/tokenstore/store_test.goRepository: DefangLabs/defang
Length of output: 4892
Add allowEqual parameter to isWithinBase to prevent operations on the store root directory.
The current implementation allows rel == "." unconditionally, which means Delete(".") would resolve to the token store directory itself. While List correctly needs to accept root operations, getTokenFile should reject paths that resolve to the store root since tokens must be files within the directory, not the directory itself.
The proposed fix correctly introduces an allowEqual parameter to differentiate these cases: true for List (to allow listing the root directory) and false for getTokenFile (to prevent targeting the store root as a token key).
Proposed fix
-func isWithinBase(baseDir, target string) bool {
+func isWithinBase(baseDir, target string, allowEqual bool) bool {
rel, err := filepath.Rel(baseDir, target)
if err != nil {
return false
}
- return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) && !filepath.IsAbs(rel))
+ if rel == "." {
+ return allowEqual
+ }
+ return rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) && !filepath.IsAbs(rel)
}
...
- if !isWithinBase(baseDir, dir) {
+ if !isWithinBase(baseDir, dir, true) {
...
- if !isWithinBase(absDir, absTokenFilePath) {
+ if !isWithinBase(absDir, absTokenFilePath, false) {
return "", errors.New("invalid token key")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/tokenstore/store.go` around lines 61 - 67, isWithinBase currently
treats rel == "." as within base which allows operations on the store root;
change its signature to isWithinBase(baseDir, target string, allowEqual bool)
and adjust the return so rel == "." is considered inside only when allowEqual is
true (i.e., return allowEqual || rel != "." and keep the existing checks for
".." and absolute paths). Update all call sites: have List call
isWithinBase(..., true) to permit root listing, and have getTokenFile (and any
delete/token-modifying paths) call isWithinBase(..., false) so operations that
resolve to the store root are rejected. Ensure error handling and tests reflect
the new behavior.
jordanstephens
changed the title
Converts single-line informational messages from term.Println/Printf to the appropriate slog level (Info, Warn, Debug, or Error). Data output, terminal-control operations, interactive prompts, and multi-line formatted instructions are left as term calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/cli/command/compose.go (1)
449-479:⚠️ Potential issue | 🔴 CriticalGuard
listConfigsbefore dereferencing afterListConfigfailures.At Line 478,
len(listConfigs.Names)can panic whenListConfigfails (or returns a nil response). This is a runtime crash path incompose down.💡 Proposed fix
- listConfigs, err := session.Provider.ListConfig(cmd.Context(), &defangv1.ListConfigsRequest{Project: projectName}) - if err == nil { - if len(listConfigs.Names) > 0 { - slog.Warn("Stored project configs are not deleted.") - } - } else { - slog.Debug(fmt.Sprintf("ListConfigs failed: %v", err)) - } + listConfigs, err := session.Provider.ListConfig(cmd.Context(), &defangv1.ListConfigsRequest{Project: projectName}) + configNames := []string{} + if err != nil { + slog.Debug(fmt.Sprintf("ListConfigs failed: %v", err)) + } else if listConfigs != nil { + configNames = listConfigs.Names + if len(configNames) > 0 { + slog.Warn("Stored project configs are not deleted.") + } + } @@ - if len(listConfigs.Names) > 0 { - printDefangHint("To delete stored project configs, run:", "config rm --project-name="+projectName+" "+strings.Join(listConfigs.Names, " ")) + if len(configNames) > 0 { + printDefangHint("To delete stored project configs, run:", "config rm --project-name="+projectName+" "+strings.Join(configNames, " ")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/compose.go` around lines 449 - 479, The code dereferences listConfigs.Names after calling session.Provider.ListConfig and can panic if ListConfig returned an error or nil response; update the usage to guard against nil and only check length when listConfigs != nil (e.g., replace checks like len(listConfigs.Names) > 0 with listConfigs != nil && len(listConfigs.Names) > 0), or initialize listConfigs to an empty defangv1.ListConfigsResponse when ListConfig fails; ensure the first warning/condition and the final printDefangHint block both use this nil-safe guard so ListConfig, listConfigs, and printDefangHint are not dereferenced on error.
🧹 Nitpick comments (3)
src/pkg/clouds/aws/login.go (1)
296-296: Remove trailing newline from structured log message.Line 296 appends
\ninsideslog.Debug; structured logs should not include manual newline terminators.♻️ Proposed tweak
- slog.Debug(fmt.Sprintf("token %q is valid and can assume AWS_PROFILE role %q\n", name, roleArn)) + slog.Debug(fmt.Sprintf("token %q is valid and can assume AWS_PROFILE role %q", name, roleArn))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` at line 296, The log call uses fmt.Sprintf with a trailing "\n" inside slog.Debug; remove the trailing newline from the formatted string (i.e., drop "\n" from the fmt.Sprintf) and preferably convert the call to structured logging by passing keys and values to slog.Debug instead of a formatted string—locate the slog.Debug(fmt.Sprintf(...)) invocation (the fmt.Sprintf token %q usage for variables name and roleArn) and update it to not include "\n" or to use slog.Debug("message", "name", name, "roleArn", roleArn).src/pkg/cli/new.go (1)
39-39: Use structured slog fields instead of pre-formatted messages for better logging.Lines 39, 72, 104, and 117 use pre-formatted strings with
fmt.Sprintfor string concatenation. This loses structure and performs unnecessary formatting upfront. Refactor to use structured key-value fields:♻️ Suggested refactor
- slog.Debug(fmt.Sprintf("%v", resp.Header)) + slog.Debug("Fetched sample index response", "headers", resp.Header) - slog.Debug(fmt.Sprintf("%v", resp.Header)) + slog.Debug("Fetched samples tarball response", "headers", resp.Header) - slog.Info(" - " + base) + slog.Info("Copying sample file", "file", base) - slog.Warn(fmt.Sprintf("File already exists, skipping: %q", path)) + slog.Warn("File already exists, skipping", "path", path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/new.go` at line 39, Replace pre-formatted slog messages with structured key/value fields: when you see slog.Debug(fmt.Sprintf("%v", resp.Header)) and other places using fmt.Sprintf or string concatenation in src/pkg/cli/new.go, change them to use structured fields such as slog.Debug("http response", slog.Any("header", resp.Header)) or slog.Debug("message", slog.String("details", value)) as appropriate; update each occurrence (the debug calls that currently build strings from resp.Header and other values) to pass a short message and the data as keyed fields using slog.Any/slog.String/slog.Int/etc. so logs remain structured and avoid unnecessary fmt.Sprintf.src/pkg/clouds/gcp/login.go (1)
114-114: Replacefmt.Sprintfwith structured slog fields.Multiple slog calls in this file use
fmt.Sprintfto pre-format strings (including some with embedded newlines), which reduces log queryability and performs unnecessary formatting work during filtering. Use structured fields instead:
- Lines 114, 119, 131, 135, 150, 174, 176, 225, 230, 233, 236, 242, 258, 269
Example refactor pattern
- slog.Debug(fmt.Sprintf("ACTIONS_ID_TOKEN_REQUEST_URL=%q, GOOGLE_WORKLOAD_IDENTITY_PROVIDER=%q", githubTokenReqUrl, gcpProvider)) + slog.Debug("GitHub Actions env detected", + "ACTIONS_ID_TOKEN_REQUEST_URL", githubTokenReqUrl, + "GOOGLE_WORKLOAD_IDENTITY_PROVIDER", gcpProvider, + ) - slog.Warn(fmt.Sprintf("failed to load stored credentials: %v", err)) + slog.Warn("failed to load stored credentials", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/login.go` at line 114, Replace calls that pre-format messages with fmt.Sprintf in the slog.Debug/Slog calls (e.g., the call that logs os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") and the other listed lines) by emitting structured slog records with fields instead of interpolated strings: locate each slog.Debug/Info call that currently wraps fmt.Sprintf (starting with the one referencing os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") and the occurrences at lines 119, 131, 135, 150, 174, 176, 225, 230, 233, 236, 242, 258, 269) and change them to use slog functions that pass key/value attributes (e.g., slog.String("env.GOOGLE_APPLICATION_CREDENTIALS", os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")), slog.Bool("hasPermission", ok), slog.String("reason", err.Error()), etc.) so logs are queryable and avoid unnecessary string formatting; keep the same log level and message context but move data into structured fields and remove fmt.Sprintf usage in the Cloud GCP login-related functions where those slog calls appear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/clouds/gcp/login.go`:
- Line 330: The interactive login instruction currently uses slog.Info and
should be printed directly to the terminal so it isn't suppressed or formatted
by the logger; replace the slog.Info("Please visit the following URL...") call
in login.go with a direct terminal write (e.g., fmt.Fprintln(os.Stderr, "Please
visit the following URL to log in to Google Cloud Platform: (Right click the URL
or press ENTER to open browser)")) and add imports for fmt and os if missing;
keep the exact message text and ensure this occurs in the same function where
the slog.Info call exists so users always see the prompt during interactive
auth.
---
Outside diff comments:
In `@src/cmd/cli/command/compose.go`:
- Around line 449-479: The code dereferences listConfigs.Names after calling
session.Provider.ListConfig and can panic if ListConfig returned an error or nil
response; update the usage to guard against nil and only check length when
listConfigs != nil (e.g., replace checks like len(listConfigs.Names) > 0 with
listConfigs != nil && len(listConfigs.Names) > 0), or initialize listConfigs to
an empty defangv1.ListConfigsResponse when ListConfig fails; ensure the first
warning/condition and the final printDefangHint block both use this nil-safe
guard so ListConfig, listConfigs, and printDefangHint are not dereferenced on
error.
---
Nitpick comments:
In `@src/pkg/cli/new.go`:
- Line 39: Replace pre-formatted slog messages with structured key/value fields:
when you see slog.Debug(fmt.Sprintf("%v", resp.Header)) and other places using
fmt.Sprintf or string concatenation in src/pkg/cli/new.go, change them to use
structured fields such as slog.Debug("http response", slog.Any("header",
resp.Header)) or slog.Debug("message", slog.String("details", value)) as
appropriate; update each occurrence (the debug calls that currently build
strings from resp.Header and other values) to pass a short message and the data
as keyed fields using slog.Any/slog.String/slog.Int/etc. so logs remain
structured and avoid unnecessary fmt.Sprintf.
In `@src/pkg/clouds/aws/login.go`:
- Line 296: The log call uses fmt.Sprintf with a trailing "\n" inside
slog.Debug; remove the trailing newline from the formatted string (i.e., drop
"\n" from the fmt.Sprintf) and preferably convert the call to structured logging
by passing keys and values to slog.Debug instead of a formatted string—locate
the slog.Debug(fmt.Sprintf(...)) invocation (the fmt.Sprintf token %q usage for
variables name and roleArn) and update it to not include "\n" or to use
slog.Debug("message", "name", name, "roleArn", roleArn).
In `@src/pkg/clouds/gcp/login.go`:
- Line 114: Replace calls that pre-format messages with fmt.Sprintf in the
slog.Debug/Slog calls (e.g., the call that logs
os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") and the other listed lines) by
emitting structured slog records with fields instead of interpolated strings:
locate each slog.Debug/Info call that currently wraps fmt.Sprintf (starting with
the one referencing os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") and the
occurrences at lines 119, 131, 135, 150, 174, 176, 225, 230, 233, 236, 242, 258,
269) and change them to use slog functions that pass key/value attributes (e.g.,
slog.String("env.GOOGLE_APPLICATION_CREDENTIALS",
os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")), slog.Bool("hasPermission", ok),
slog.String("reason", err.Error()), etc.) so logs are queryable and avoid
unnecessary string formatting; keep the same log level and message context but
move data into structured fields and remove fmt.Sprintf usage in the Cloud GCP
login-related functions where those slog calls appear.
🪄 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: 8964106e-0685-47e6-a980-36fbdc987b98
📒 Files selected for processing (15)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/compose.gosrc/cmd/cli/command/compose_test.gosrc/cmd/cli/command/mcp.gosrc/pkg/auth/auth.gosrc/pkg/cli/cd.gosrc/pkg/cli/compose/loader.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/generate.gosrc/pkg/cli/new.gosrc/pkg/clouds/aws/login.gosrc/pkg/clouds/gcp/api.gosrc/pkg/clouds/gcp/login.gosrc/pkg/debug/debug.gosrc/pkg/login/agree_tos.go
✅ Files skipped from review due to trivial changes (3)
- src/pkg/cli/compose/loader.go
- src/pkg/auth/auth.go
- src/pkg/clouds/gcp/api.go
🚧 Files skipped from review as they are similar to previous changes (6)
- src/cmd/cli/command/compose_test.go
- src/pkg/cli/generate.go
- src/pkg/login/agree_tos.go
- src/pkg/cli/composeUp.go
- src/pkg/cli/cd.go
- src/cmd/cli/command/mcp.go
lionello
requested changes
Apr 21, 2026
| rangeStr += " until " + untilTs.Format(time.RFC3339Nano) | ||
| } | ||
| term.Infof("Showing logs%s; press Ctrl+C to stop:", rangeStr) | ||
| slog.Info(fmt.Sprintf("Showing logs%s; press Ctrl+C to stop:", rangeStr)) |
Member
There was a problem hiding this comment.
This pattern is so obiquitous, we should consider making our own log.Infof, log.Warnf, log.Errorf etc.
Member
There was a problem hiding this comment.
Alternatively, we could use the log stdlib packages, instead of slog, which is more term-like (no key-value). This still allows service consuming defang CLI packages (fabric) to hook it up to their own logger (ie. slog).
Member
Author
There was a problem hiding this comment.
That might make more sense tbh
…y from term.Println Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pkg/cli/generate.go (1)
57-63:⚠️ Potential issue | 🔴 CriticalPrevent path traversal when writing generated files.
At Line 61,
file.Nameis joined directly into the destination path. A generated name like../...or an absolute path can escapeargs.Folderand overwrite unintended files.🔒 Proposed hardening
import ( "context" "fmt" "log/slog" "os" "path/filepath" + "strings" @@ for _, file := range response.Files { // Print the files that were generated fmt.Println(" -", file.Name) - // TODO: this will overwrite existing files - if err = os.WriteFile(filepath.Join(args.Folder, file.Name), []byte(file.Content), 0644); err != nil { - return nil, err + + cleanName := filepath.Clean(file.Name) + if cleanName == "." || filepath.IsAbs(cleanName) || strings.HasPrefix(cleanName, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("invalid generated file path %q", file.Name) + } + + dst := filepath.Join(args.Folder, cleanName) + if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { + return nil, fmt.Errorf("create parent directory for %q: %w", dst, err) + } + if err = os.WriteFile(dst, []byte(file.Content), 0644); err != nil { + return nil, fmt.Errorf("write generated file %q: %w", dst, err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/generate.go` around lines 57 - 63, The loop writing response.Files uses file.Name directly which allows path traversal or absolute paths to escape args.Folder; sanitize and validate each file destination before writing: reject absolute paths (filepath.IsAbs), clean the joined path (filepath.Clean(filepath.Join(args.Folder, file.Name))) then ensure the cleaned path is contained within args.Folder (e.g., via filepath.Rel and verifying it doesn't start with ".."); create any required parent directories (os.MkdirAll) and only then call os.WriteFile to write the content; if validation fails return an error instead of writing.src/pkg/clouds/aws/login.go (1)
78-91: 🛠️ Refactor suggestion | 🟠 MajorReplace
fmt.Sprintfwith structured slog fields throughout the file.This file has 19 instances where
fmt.Sprintfpreformats messages insideslog.Debug()andslog.WarnContext()calls. Preformatted strings eliminate structured logging benefits and waste CPU on string formatting when logs are disabled. Instead, pass the message and attributes separately:slog.Debug("message", "key", value).Examples: line 78 should be
slog.Debug("failed to refresh AWS OAuth token", "error", err), line 89 should beslog.WarnContext(ctx, "failed to persist refreshed AWS OAuth token", "error", err), line 130 should beslog.Debug("checking default AWS credentials", "region", a.Region).This applies throughout the file (lines 78, 89, 91, 130, 135, 183, 191, 209, 215, 225, 229, 240, 260, 266, 268, 272, 292, 296, 342).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` around lines 78 - 91, Several log calls in this file pre-format messages with fmt.Sprintf (e.g., in the token refresh/persist block around p.tokenStore, p.storeKey and variable refreshed) which defeats structured slog usage and wastes CPU; replace each fmt.Sprintf usage passed into slog.Debug, slog.WarnContext, etc., with structured slog calls that supply a short literal message and key/value attributes (for example use slog.Debug("failed to refresh AWS OAuth token", "error", err) and slog.WarnContext(ctx, "failed to persist refreshed AWS OAuth token", "error", err)); update all the listed locations (including uses around symbols p.tokenStore, p.storeKey, refreshed and other checks like the default credentials/debugging references such as a.Region) to follow the same pattern so messages are not preformatted and use attribute keys like "error", "region", "storeKey" as appropriate.
🧹 Nitpick comments (7)
src/pkg/cli/client/byoc/state/parse.go (1)
89-94: Use structuredslogfields withDebugContextinstead of preformattedfmt.Sprintmessages.Lines 89 and 94 currently use
fmt.Sprintto concatenate log strings, which makes logs less queryable. Sincectxis available in the function signature, refactor to useslog.DebugContextwith key/value attributes:Suggested changes
- if state.Version != 3 { - slog.Debug(fmt.Sprint("Skipping Pulumi state with version", state.Version)) + if state.Version != 3 { + slog.DebugContext(ctx, "Skipping Pulumi state with unsupported version", "version", state.Version) } else if len(state.Checkpoint.Latest.PendingOperations) > 0 { for _, op := range state.Checkpoint.Latest.PendingOperations { parts := strings.Split(op.Resource.Urn, "::") // prefix::project::type::resource => {urn:provider:stack}::{project}::{plugin:file:class}::{name} if len(parts) < 4 { - slog.Debug(fmt.Sprint("Skipping pending operation with malformed URN:", op.Resource.Urn)) + slog.DebugContext(ctx, "Skipping pending operation with malformed URN", "urn", op.Resource.Urn) continue } stack.Pending = append(stack.Pending, parts[3]) }This follows the coding guideline for
**/*.go: Use log/slog for structured debug logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/state/parse.go` around lines 89 - 94, Replace the preformatted fmt.Sprint logging calls with structured slog DebugContext calls using the available ctx: instead of slog.Debug(fmt.Sprint("Skipping Pulumi state with version", state.Version)) call slog.DebugContext(ctx, "Skipping Pulumi state with version", "version", state.Version), and instead of slog.Debug(fmt.Sprint("Skipping pending operation with malformed URN:", op.Resource.Urn)) call slog.DebugContext(ctx, "Skipping pending operation with malformed URN", "urn", op.Resource.Urn); update the two occurrences that reference state.Version and op.Resource.Urn respectively so logs use key/value attributes for better queryability.src/pkg/cli/generate.go (1)
37-39: Wrap returned errors with operation context.These branches return raw errors, which makes troubleshooting harder once they bubble up. Add
fmt.Errorf(... %w)with operation-specific context.🧭 Proposed error-context wrapping
response, err := client.GenerateFiles(ctx, &defangv1.GenerateFilesRequest{ @@ }) if err != nil { - return nil, err + return nil, fmt.Errorf("generate files from API: %w", err) } @@ if err := os.MkdirAll(args.Folder, 0755); err != nil { - return nil, err + return nil, fmt.Errorf("create output folder %q: %w", args.Folder, err) } @@ - if err = os.WriteFile(filepath.Join(args.Folder, file.Name), []byte(file.Content), 0644); err != nil { - return nil, err + if err = os.WriteFile(filepath.Join(args.Folder, file.Name), []byte(file.Content), 0644); err != nil { + return nil, fmt.Errorf("write generated file %q: %w", file.Name, err) } }As per coding guidelines "Wrap errors with context using fmt.Errorf with %w verb".
Also applies to: 54-56, 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/generate.go` around lines 37 - 39, Replace bare returns of err in src/pkg/cli/generate.go (the occurrences that currently do "return nil, err" and "return err" around the shown branches and the other two spots at lines 54-56 and 61-63) with wrapped errors using fmt.Errorf to add operation-specific context; e.g. change to return nil, fmt.Errorf("failed to <describe operation e.g. create/generate X>: %w", err) (or return fmt.Errorf("failed to <operation>: %w", err)) and add an import for fmt if missing so every error that bubbles up includes the operation context.src/pkg/stacks/manager.go (1)
111-111: Remove trailing newline from slog message.The
\nat the end of the format string is unnecessary—slog handlers typically append newlines automatically. This could result in double newlines in some output handlers.Suggested fix
- slog.WarnContext(ctx, fmt.Sprintf("Skipping invalid remote stack %s: %v\n", name, err)) + slog.WarnContext(ctx, fmt.Sprintf("Skipping invalid remote stack %s: %v", name, err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/stacks/manager.go` at line 111, The slog warning call in manager.go uses fmt.Sprintf with a trailing "\n" which can cause double newlines in output handlers; edit the slog.WarnContext invocation that formats the message for the invalid remote stack (the call that mentions "Skipping invalid remote stack" and uses fmt.Sprintf) and remove the trailing "\n" from the format string so the message does not include an explicit newline.src/pkg/cli/composeUp.go (1)
123-123: Consider using context-aware slog variants for consistency.Lines 123 and 212 use
slog.Debugwhile line 213 correctly usesslog.WarnContext. Sincectxis available inComposeUp, consider usingslog.DebugContext(ctx, ...)for consistency with the rest of the migration.Suggested fix
- slog.Debug(fmt.Sprint("GetDelegateSubdomainZone failed:", err)) + slog.DebugContext(ctx, fmt.Sprint("GetDelegateSubdomainZone failed:", err))- slog.Debug(fmt.Sprint("Failed to record deployment:", err)) + slog.DebugContext(ctx, fmt.Sprint("Failed to record deployment:", err))Also applies to: 212-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/composeUp.go` at line 123, The call sites in ComposeUp using slog.Debug (notably the call with message "GetDelegateSubdomainZone failed:" and the other at lines around 212) should use context-aware logging to match existing slog.WarnContext usage; update those invocations in ComposeUp to slog.DebugContext(ctx, ...) (passing the same ctx already in scope) and include err as a structured field or formatted message consistent with slog.WarnContext usage so logs remain context-aware and uniform.src/cmd/cli/command/stack.go (1)
96-96: Remove trailing newlines from slog messages.Both format strings include
\nwhich is unnecessary—slog handlers typically handle line termination. This could result in double newlines in output.Suggested fix
- slog.Debug(fmt.Sprintf("Creating stack with parameters: %+v\n", params)) + slog.Debug(fmt.Sprintf("Creating stack with parameters: %+v", params))- slog.InfoContext(ctx, fmt.Sprintf("Stack %q is now the default stack for project %q\n", name, projectName)) + slog.InfoContext(ctx, fmt.Sprintf("Stack %q is now the default stack for project %q", name, projectName))Also applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/stack.go` at line 96, Remove the trailing "\n" from the fmt.Sprintf format strings passed to slog.Debug so the logger controls line termination; locate the slog.Debug(...) calls that use fmt.Sprintf("Creating stack with parameters: %+v\n", params) (and the similar call at the second occurrence around the other slog.Debug) and change the format to "Creating stack with parameters: %+v" (remove the newline) while keeping the rest of the message and parameters unchanged.src/pkg/cli/cd.go (1)
84-84: Consider using context-aware slog variants for consistency.These
slog.Debugcalls are in functions wherectxis available. For consistency with the rest of the migration (which usesslog.DebugContextin similar situations), consider using the context-aware variant.Suggested fix
- slog.Debug(fmt.Sprint("DeleteSubdomainZone failed:", err)) + slog.DebugContext(ctx, fmt.Sprint("DeleteSubdomainZone failed:", err))- slog.Debug(fmt.Sprint("Tail stopped with", err, errors.Unwrap(err))) + slog.DebugContext(ctx, fmt.Sprint("Tail stopped with", err, errors.Unwrap(err)))- slog.Debug("Running CD list") + slog.DebugContext(ctx, "Running CD list")Also applies to: 125-125, 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/cd.go` at line 84, Replace plain slog.Debug calls with the context-aware slog.DebugContext variant and pass the available ctx to keep logging consistent; e.g., change slog.Debug(fmt.Sprint("DeleteSubdomainZone failed:", err)) to slog.DebugContext(ctx, fmt.Sprint("DeleteSubdomainZone failed:", err)) and do the same for the other two occurrences (the slog.Debug calls around lines 125 and 140) so they use slog.DebugContext(ctx, ...) and preserve the same message and error formatting.src/pkg/auth/auth.go (1)
76-76: Replacefmt.Sprintfwith structured fields inslogcalls for better JSON/structured log queryability.Lines 76, 82, 120, and 165 currently flatten errors, status, retry delay, and scopes into message strings. Structured logging with dedicated fields enables proper filtering and analysis in JSON handlers.
♻️ Refactor these lines
- slog.ErrorContext(ctx, fmt.Sprintf("failed to poll for auth code: %v", err)) + slog.ErrorContext(ctx, "failed to poll for auth code", "error", err) - slog.ErrorContext(ctx, fmt.Sprintf("failed to exchange code for token: %v", err)) + slog.ErrorContext(ctx, "failed to exchange code for token", "error", err) - slog.Debug(fmt.Sprintf("received server error: %s, retrying in %v...", unexpectedError.Status, retryDelay)) + slog.Debug("received server error, retrying", "status", unexpectedError.Status, "retryDelay", retryDelay) - slog.Debug(fmt.Sprintf("Generating access token with scopes %v", scopes)) + slog.Debug("generating access token", "scopes", scopes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/auth/auth.go` at line 76, Replace the fmt.Sprintf-based message concatenation in slog calls with structured key/value fields: update the slog.ErrorContext invocations (e.g., the one currently doing slog.ErrorContext(ctx, fmt.Sprintf("failed to poll for auth code: %v", err))) to call slog.ErrorContext(ctx, "failed to poll for auth code", "err", err) and likewise replace the other flattened messages to pass explicit fields such as "status", status, "retry_delay", retryDelay, and "scopes", scopes so that each log call uses a static message plus structured fields for the error and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/mcp.go`:
- Around line 63-68: Replace the direct fmt.Println calls in the MCP command
around server.ServeStdio(s) with the project terminal wrapper from pkg/term so
user-facing output goes through the CLI terminal abstraction; specifically,
remove fmt.Println("Starting Defang MCP server") and fmt.Println("Server
shutdown") and call the appropriate pkg/term output function (e.g.,
term.Info/term.Println/term.Printf per the pkg/term API) so the messages use
colors/spinners and consistent output, and update imports to remove fmt and add
pkg/term.
In `@src/pkg/cli/client/byoc/aws/byoc.go`:
- Line 276: The debug log emits the Docker Hub username via
slog.Debug(fmt.Sprintf("Using Docker Hub credentials with user %v",
dockerHubUser)), which may expose credential-linked identifiers; change this to
avoid printing dockerHubUser directly — either remove the username from the
message (e.g., log a generic "Using Docker Hub credentials" or include a
non-identifying tag) or redact it before logging (e.g., replace with a masked
value or only log a user-length/hashed indicator) and update the slog.Debug call
accordingly.
In `@src/pkg/cli/client/byoc/do/byoc.go`:
- Line 113: Replace string-built debug logs that use fmt.Sprint/fmt.Sprintf with
structured slog calls: where you currently call
slog.Debug(fmt.Sprint("s3.GetObject:", err)) (and the similar uses around s3
operations), change to slog.DebugContext(ctx, "s3.GetObject failed", "error",
err) (or an appropriate message) so the context and error are passed as
key/value attributes; ensure you import/use the existing ctx variable and
replace other fmt-based slog.Debug calls analogously (e.g., use
slog.DebugContext(ctx, "operation", "error", err, "bucket", bucketName) to add
relevant fields).
In `@src/pkg/cli/compose/context.go`:
- Line 487: The warning currently interpolates buf.Len() which shows the current
buffer size rather than the configured soft-limit; update the slog.WarnContext
call in the context creation logic (the call site using buf.Len()) to print the
configured threshold variable (e.g., softLimit / maxContextSize /
configuredThreshold) formatted via units.BytesSize(float64(threshold)) so the
message reports the intended soft-limit value instead of the runtime buffer
length.
- Line 223: Fix malformed slog message construction and use the appropriate
context-aware variant: in getRemoteBuildContext replace fmt.Sprint/Printf
concatenations with slog.DebugContext(ctx, ...) using either a formatted string
with proper spacing or, better, structured key/value pairs (e.g.,
slog.DebugContext(ctx, "packaging project", "service", service, "root", root))
for the calls currently using slog.InfoContext/slog.DebugContext on lines
referencing packaging and debug inside getRemoteBuildContext; for functions or
callbacks that do NOT receive a ctx param (the other slog calls in this file),
switch to slog.Debug(...) and emit structured key/value pairs instead of
concatenated strings (e.g., slog.Debug("event", "something", "service", service,
"root", root)); ensure all concatenations like fmt.Sprint("Packaging the project
files for", service, "at", root) are replaced with properly spaced formatting or
structured attributes and convert the one debug call mentioned to use
DebugContext when ctx is available.
In `@src/pkg/mcp/mcp_server.go`:
- Around line 33-39: Replace the string-concatenated slog calls with structured
logging using DebugContext/ErrorContext: call slog.DebugContext(ctx, ...) before
invoking handler to log the event with key-value pairs like "tool": name,
"params": request.Params, "client": t.client, "cluster": t.fabricAddr,
"provider": *t.providerId (use the existing Debug call site), and after handler
returns call slog.DebugContext(ctx, "MCP Tool Succeeded", "tool", name) on
success and slog.ErrorContext(ctx, "MCP Tool Failed", "tool", name, "error",
err, "client", t.client, "cluster", t.fabricAddr, "provider", *t.providerId) on
error; avoid fmt.Sprint/fmt.Sprintf for building log messages so structured
attributes are preserved.
In `@src/pkg/mcp/resources/resources.go`:
- Line 41: The current calls wrap structured key/value pairs in fmt.Sprint which
collapses them into a single message and destroys structured logging; update the
slog.ErrorContext invocations (e.g., the call using fmt.Sprint at the
slog.ErrorContext(ctx, fmt.Sprint(...)) lines that mention "Failed to read
resource file" and the occurrence around "knowledge_base.json") to pass a short
message string as the first arg and the key/value pairs directly afterwards (for
example: slog.ErrorContext(ctx, "Failed to read resource file", "error", err,
"path", "knowledge_base.json")), removing the fmt.Sprint wrapper so error and
path remain separate structured fields.
In `@src/pkg/migrate/heroku.go`:
- Around line 329-331: The code returns raw err in multiple branches (e.g.,
after slog.Debug(fmt.Sprintf("Failed to run `heroku login`: %v", err)) and
similar blocks at the noted ranges), losing call-site context; change each plain
return err to return fmt.Errorf("failed to run heroku login: %w", err) (or an
equivalent contextual message matching the operation in that branch), and ensure
you import fmt if not present; apply the same wrapping pattern to the other
branches mentioned (lines 350-354, 359-362, 371-375) so all CLI/parsing errors
are wrapped with context using the %w verb.
In `@src/pkg/migrate/migrate.go`:
- Line 78: The debug log prints raw applicationInfo before secret redaction;
remove the direct slog.Debug(fmt.Sprintf("Application info: %+v\n",
applicationInfo)) call or replace it with a log of the already-sanitized
structure (e.g., the redactedApplicationInfo variable or the output of the
existing sanitization function) so secrets are never emitted. Locate the
slog.Debug call that references applicationInfo in migrate.go and either delete
it or change it to log only the sanitized/redacted data (or a minimal
non-sensitive summary).
---
Outside diff comments:
In `@src/pkg/cli/generate.go`:
- Around line 57-63: The loop writing response.Files uses file.Name directly
which allows path traversal or absolute paths to escape args.Folder; sanitize
and validate each file destination before writing: reject absolute paths
(filepath.IsAbs), clean the joined path
(filepath.Clean(filepath.Join(args.Folder, file.Name))) then ensure the cleaned
path is contained within args.Folder (e.g., via filepath.Rel and verifying it
doesn't start with ".."); create any required parent directories (os.MkdirAll)
and only then call os.WriteFile to write the content; if validation fails return
an error instead of writing.
In `@src/pkg/clouds/aws/login.go`:
- Around line 78-91: Several log calls in this file pre-format messages with
fmt.Sprintf (e.g., in the token refresh/persist block around p.tokenStore,
p.storeKey and variable refreshed) which defeats structured slog usage and
wastes CPU; replace each fmt.Sprintf usage passed into slog.Debug,
slog.WarnContext, etc., with structured slog calls that supply a short literal
message and key/value attributes (for example use slog.Debug("failed to refresh
AWS OAuth token", "error", err) and slog.WarnContext(ctx, "failed to persist
refreshed AWS OAuth token", "error", err)); update all the listed locations
(including uses around symbols p.tokenStore, p.storeKey, refreshed and other
checks like the default credentials/debugging references such as a.Region) to
follow the same pattern so messages are not preformatted and use attribute keys
like "error", "region", "storeKey" as appropriate.
---
Nitpick comments:
In `@src/cmd/cli/command/stack.go`:
- Line 96: Remove the trailing "\n" from the fmt.Sprintf format strings passed
to slog.Debug so the logger controls line termination; locate the
slog.Debug(...) calls that use fmt.Sprintf("Creating stack with parameters:
%+v\n", params) (and the similar call at the second occurrence around the other
slog.Debug) and change the format to "Creating stack with parameters: %+v"
(remove the newline) while keeping the rest of the message and parameters
unchanged.
In `@src/pkg/auth/auth.go`:
- Line 76: Replace the fmt.Sprintf-based message concatenation in slog calls
with structured key/value fields: update the slog.ErrorContext invocations
(e.g., the one currently doing slog.ErrorContext(ctx, fmt.Sprintf("failed to
poll for auth code: %v", err))) to call slog.ErrorContext(ctx, "failed to poll
for auth code", "err", err) and likewise replace the other flattened messages to
pass explicit fields such as "status", status, "retry_delay", retryDelay, and
"scopes", scopes so that each log call uses a static message plus structured
fields for the error and metadata.
In `@src/pkg/cli/cd.go`:
- Line 84: Replace plain slog.Debug calls with the context-aware
slog.DebugContext variant and pass the available ctx to keep logging consistent;
e.g., change slog.Debug(fmt.Sprint("DeleteSubdomainZone failed:", err)) to
slog.DebugContext(ctx, fmt.Sprint("DeleteSubdomainZone failed:", err)) and do
the same for the other two occurrences (the slog.Debug calls around lines 125
and 140) so they use slog.DebugContext(ctx, ...) and preserve the same message
and error formatting.
In `@src/pkg/cli/client/byoc/state/parse.go`:
- Around line 89-94: Replace the preformatted fmt.Sprint logging calls with
structured slog DebugContext calls using the available ctx: instead of
slog.Debug(fmt.Sprint("Skipping Pulumi state with version", state.Version)) call
slog.DebugContext(ctx, "Skipping Pulumi state with version", "version",
state.Version), and instead of slog.Debug(fmt.Sprint("Skipping pending operation
with malformed URN:", op.Resource.Urn)) call slog.DebugContext(ctx, "Skipping
pending operation with malformed URN", "urn", op.Resource.Urn); update the two
occurrences that reference state.Version and op.Resource.Urn respectively so
logs use key/value attributes for better queryability.
In `@src/pkg/cli/composeUp.go`:
- Line 123: The call sites in ComposeUp using slog.Debug (notably the call with
message "GetDelegateSubdomainZone failed:" and the other at lines around 212)
should use context-aware logging to match existing slog.WarnContext usage;
update those invocations in ComposeUp to slog.DebugContext(ctx, ...) (passing
the same ctx already in scope) and include err as a structured field or
formatted message consistent with slog.WarnContext usage so logs remain
context-aware and uniform.
In `@src/pkg/cli/generate.go`:
- Around line 37-39: Replace bare returns of err in src/pkg/cli/generate.go (the
occurrences that currently do "return nil, err" and "return err" around the
shown branches and the other two spots at lines 54-56 and 61-63) with wrapped
errors using fmt.Errorf to add operation-specific context; e.g. change to return
nil, fmt.Errorf("failed to <describe operation e.g. create/generate X>: %w",
err) (or return fmt.Errorf("failed to <operation>: %w", err)) and add an import
for fmt if missing so every error that bubbles up includes the operation
context.
In `@src/pkg/stacks/manager.go`:
- Line 111: The slog warning call in manager.go uses fmt.Sprintf with a trailing
"\n" which can cause double newlines in output handlers; edit the
slog.WarnContext invocation that formats the message for the invalid remote
stack (the call that mentions "Skipping invalid remote stack" and uses
fmt.Sprintf) and remove the trailing "\n" from the format string so the message
does not include an explicit newline.
🪄 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: f8deab5e-2e4c-4cf6-89df-7ad4b2b27b41
📒 Files selected for processing (67)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/compose.gosrc/cmd/cli/command/compose_test.gosrc/cmd/cli/command/config.gosrc/cmd/cli/command/generate.gosrc/cmd/cli/command/mcp.gosrc/cmd/cli/command/session.gosrc/cmd/cli/command/stack.gosrc/cmd/cli/command/whoami.gosrc/pkg/agent/tools/listConfig.gosrc/pkg/agent/tools/logs.gosrc/pkg/auth/auth.gosrc/pkg/cli/cd.gosrc/pkg/cli/cert.gosrc/pkg/cli/client/byoc/aws/alb_logs.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/domain.gosrc/pkg/cli/client/byoc/aws/list.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/client/byoc/common.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/state/parse.gosrc/pkg/cli/client/grpc_logger.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/pretty_error.gosrc/pkg/cli/client/projectName.gosrc/pkg/cli/compose/context.gosrc/pkg/cli/compose/fixup.gosrc/pkg/cli/compose/loader.gosrc/pkg/cli/composeDown.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/configList.gosrc/pkg/cli/connect.gosrc/pkg/cli/deploymentsList.gosrc/pkg/cli/estimate.gosrc/pkg/cli/generate.gosrc/pkg/cli/install_cd.gosrc/pkg/cli/logout.gosrc/pkg/cli/new.gosrc/pkg/cli/subscribe.gosrc/pkg/cli/tail.gosrc/pkg/cli/tailAndMonitor.gosrc/pkg/cli/teardown_cd.gosrc/pkg/cli/token.gosrc/pkg/cli/whoami.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/login.gosrc/pkg/clouds/gcp/api.gosrc/pkg/clouds/gcp/iam.gosrc/pkg/clouds/gcp/login.gosrc/pkg/clouds/gcp/storage.gosrc/pkg/debug/debug.gosrc/pkg/debug/debug_test.gosrc/pkg/dockerhub/dockerhub.gosrc/pkg/login/agree_tos.gosrc/pkg/login/login.gosrc/pkg/mcp/mcp_server.gosrc/pkg/mcp/resources/resources.gosrc/pkg/mcp/utils.gosrc/pkg/migrate/heroku.gosrc/pkg/migrate/migrate.gosrc/pkg/session/session.gosrc/pkg/setup/setup.gosrc/pkg/stacks/manager.gosrc/pkg/tokenstore/store.go
✅ Files skipped from review due to trivial changes (23)
- src/pkg/cli/client/byoc/baseclient.go
- src/pkg/cli/client/pretty_error.go
- src/pkg/cli/connect.go
- src/pkg/debug/debug_test.go
- src/pkg/cli/client/byoc/aws/list.go
- src/pkg/clouds/aws/codebuild/cfn/setup.go
- src/cmd/cli/command/generate.go
- src/pkg/cli/whoami.go
- src/pkg/clouds/gcp/storage.go
- src/pkg/cli/client/grpc_logger.go
- src/pkg/cli/client/projectName.go
- src/pkg/mcp/utils.go
- src/pkg/clouds/gcp/api.go
- src/pkg/cli/estimate.go
- src/cmd/cli/command/session.go
- src/pkg/cli/client/byoc/gcp/stream.go
- src/pkg/clouds/gcp/iam.go
- src/cmd/cli/command/config.go
- src/pkg/login/login.go
- src/pkg/cli/client/byoc/common.go
- src/pkg/cli/compose/fixup.go
- src/pkg/cli/cert.go
- src/pkg/session/session.go
🚧 Files skipped from review as they are similar to previous changes (23)
- src/pkg/debug/debug.go
- src/pkg/cli/composeDown.go
- src/pkg/agent/tools/logs.go
- src/pkg/cli/client/playground.go
- src/pkg/cli/token.go
- src/pkg/login/agree_tos.go
- src/pkg/cli/install_cd.go
- src/pkg/cli/deploymentsList.go
- src/pkg/cli/compose/loader.go
- src/pkg/dockerhub/dockerhub.go
- src/pkg/cli/logout.go
- src/cmd/cli/command/commands.go
- src/pkg/cli/new.go
- src/pkg/cli/teardown_cd.go
- src/pkg/tokenstore/store.go
- src/pkg/clouds/gcp/login.go
- src/pkg/setup/setup.go
- src/pkg/agent/tools/listConfig.go
- src/pkg/cli/tail.go
- src/pkg/cli/subscribe.go
- src/pkg/cli/configList.go
- src/cmd/cli/command/compose_test.go
- src/pkg/cli/client/byoc/gcp/byoc.go
| } | ||
|
|
||
| term.Info("Packaging the project files for", service, "at", root) | ||
| slog.InfoContext(ctx, fmt.Sprint("Packaging the project files for", service, "at", root)) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify slog calls still building messages through fmt.Sprint/fmt.Sprintf
rg -nP 'slog\.(Info|Debug|Warn)(Context)?\([^)]*fmt\.Sprint(f)?\(' src/pkg/cli/compose/context.go
# Inspect non-context debug calls in this file
rg -n -C2 '\bslog\.Debug\s*\(' src/pkg/cli/compose/context.goRepository: DefangLabs/defang
Length of output: 2229
🏁 Script executed:
#!/bin/bash
# Get the function containing line 234 to verify ctx availability
sed -n '200,250p' src/pkg/cli/compose/context.go | head -60
# Get function containing line 301 to verify ctx availability
sed -n '290,320p' src/pkg/cli/compose/context.go
# Get function containing line 416 to verify ctx availability
sed -n '400,430p' src/pkg/cli/compose/context.goRepository: DefangLabs/defang
Length of output: 3682
🏁 Script executed:
#!/bin/bash
# Get walkContextFolder function signature and callback type
rg -A5 'func walkContextFolder' src/pkg/cli/compose/context.go
# Get function containing lines 451, 478, 487
sed -n '440,500p' src/pkg/cli/compose/context.go
# Get full function signature containing line 234
sed -n '170,235p' src/pkg/cli/compose/context.go | head -80Repository: DefangLabs/defang
Length of output: 4229
Fix malformed message construction in slog calls; adjust context-aware calls based on available parameters.
Lines 223, 245, 301, 310, 416, and 451 use fmt.Sprint/fmt.Sprintf without proper spacing, creating illegible log messages (e.g., for<service>at<root>). Additionally, line 234 should use slog.DebugContext where ctx is available within getRemoteBuildContext.
For lines 301, 310, 416, and 451—which occur in functions or callbacks without a ctx parameter—use slog.Debug with structured key-value pairs. Only lines 223, 234, and 245 (in getRemoteBuildContext) have ctx available and should use context-aware variants.
Proposed fix
- slog.InfoContext(ctx, fmt.Sprint("Packaging the project files for", service, "at", root))
+ slog.InfoContext(ctx, "Packaging project files", "service", service, "root", root)
- slog.Debug(fmt.Sprintf("Digest for %q: %s", service, digest))
+ slog.DebugContext(ctx, "Computed build context digest", "service", service, "digest", digest)
- slog.InfoContext(ctx, fmt.Sprint("Uploading the project files for", service))
+ slog.InfoContext(ctx, "Uploading project files", "service", service)
- slog.Debug(fmt.Sprint("Reading .dockerignore file from", ignorefile))
+ slog.Debug("Reading .dockerignore file", "file", ignorefile)
- slog.Debug(fmt.Sprint("Writing .dockerignore file to", path))
+ slog.Debug("Writing .dockerignore file", "path", path)
- slog.Debug(fmt.Sprint("Ignoring", relPath))
+ slog.Debug("Ignoring path", "path", relPath)
- slog.Debug(fmt.Sprint("Adding", slashPath))
+ slog.Debug("Adding path to archive", "path", slashPath)Lines 478 and 487 already correctly use slog.WarnContext with ctx and structured messages via fmt.Sprintf, though they could benefit from extracting the threshold values as attributes for better structured logging.
📝 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
| slog.InfoContext(ctx, fmt.Sprint("Packaging the project files for", service, "at", root)) | |
| slog.InfoContext(ctx, "Packaging project files", "service", service, "root", root) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/cli/compose/context.go` at line 223, Fix malformed slog message
construction and use the appropriate context-aware variant: in
getRemoteBuildContext replace fmt.Sprint/Printf concatenations with
slog.DebugContext(ctx, ...) using either a formatted string with proper spacing
or, better, structured key/value pairs (e.g., slog.DebugContext(ctx, "packaging
project", "service", service, "root", root)) for the calls currently using
slog.InfoContext/slog.DebugContext on lines referencing packaging and debug
inside getRemoteBuildContext; for functions or callbacks that do NOT receive a
ctx param (the other slog calls in this file), switch to slog.Debug(...) and
emit structured key/value pairs instead of concatenated strings (e.g.,
slog.Debug("event", "something", "service", service, "root", root)); ensure all
concatenations like fmt.Sprint("Packaging the project files for", service, "at",
root) are replaced with properly spaced formatting or structured attributes and
convert the one debug call mentioned to use DebugContext when ctx is available.
| } | ||
| if bufLen <= ContextSizeSoftLimit && buf.Len() > ContextSizeSoftLimit { | ||
| term.Warnf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(buf.Len()))) | ||
| slog.WarnContext(ctx, fmt.Sprintf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(buf.Len())))) |
Contributor
There was a problem hiding this comment.
Soft-limit warning should report the configured threshold, not current buffer size.
Line 487 interpolates buf.Len() in a threshold warning. This makes the message drift with runtime size instead of stating the soft limit users should target.
Proposed fix
- slog.WarnContext(ctx, fmt.Sprintf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(buf.Len()))))
+ slog.WarnContext(ctx, fmt.Sprintf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(ContextSizeSoftLimit))))📝 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
| slog.WarnContext(ctx, fmt.Sprintf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(buf.Len())))) | |
| slog.WarnContext(ctx, fmt.Sprintf("the build context is larger than %s; use --debug or create .dockerignore to exclude caches and build artifacts", units.BytesSize(float64(ContextSizeSoftLimit)))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/cli/compose/context.go` at line 487, The warning currently
interpolates buf.Len() which shows the current buffer size rather than the
configured soft-limit; update the slog.WarnContext call in the context creation
logic (the call site using buf.Len()) to print the configured threshold variable
(e.g., softLimit / maxContextSize / configuredThreshold) formatted via
units.BytesSize(float64(threshold)) so the message reports the intended
soft-limit value instead of the runtime buffer length.
| file, err := os.ReadFile(knowledgeBasePath) | ||
| if err != nil { | ||
| term.Error("Failed to read resource file", "error", err, "path", "knowledge_base.json") | ||
| slog.ErrorContext(ctx, fmt.Sprint("Failed to read resource file", "error", err, "path", "knowledge_base.json")) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ErrorContext calls that incorrectly wrap key/value attrs in fmt.Sprint/Sprintf
rg -nP 'slog\.ErrorContext\([^)]*fmt\.S(print|printf)\(' src/pkg/mcp/resources/resources.go src/pkg/mcp/mcp_server.goRepository: DefangLabs/defang
Length of output: 475
Pass structured key/value args directly to ErrorContext; remove fmt.Sprint() wrapping.
Lines 41 and 71 use fmt.Sprint(...) to collapse key/value pairs into a single message string. This breaks structured logging by losing individual error and path attributes. Instead, pass arguments directly:
Proposed fix
- slog.ErrorContext(ctx, fmt.Sprint("Failed to read resource file", "error", err, "path", "knowledge_base.json"))
+ slog.ErrorContext(ctx, "Failed to read resource file", "error", err, "path", "knowledge_base.json")- slog.ErrorContext(ctx, fmt.Sprint("Failed to read resource file", "error", err, "path", "samples_examples.json"))
+ slog.ErrorContext(ctx, "Failed to read resource file", "error", err, "path", "samples_examples.json")📝 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
| slog.ErrorContext(ctx, fmt.Sprint("Failed to read resource file", "error", err, "path", "knowledge_base.json")) | |
| slog.ErrorContext(ctx, "Failed to read resource file", "error", err, "path", "knowledge_base.json") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/mcp/resources/resources.go` at line 41, The current calls wrap
structured key/value pairs in fmt.Sprint which collapses them into a single
message and destroys structured logging; update the slog.ErrorContext
invocations (e.g., the call using fmt.Sprint at the slog.ErrorContext(ctx,
fmt.Sprint(...)) lines that mention "Failed to read resource file" and the
occurrence around "knowledge_base.json") to pass a short message string as the
first arg and the key/value pairs directly afterwards (for example:
slog.ErrorContext(ctx, "Failed to read resource file", "error", err, "path",
"knowledge_base.json")), removing the fmt.Sprint wrapper so error and path
remain separate structured fields.
Comment on lines
329
to
331
| if err != nil { | ||
| term.Debugf("Failed to run `heroku login`: %v", err) | ||
| slog.Debug(fmt.Sprintf("Failed to run `heroku login`: %v", err)) | ||
| return err |
Contributor
There was a problem hiding this comment.
Wrap returned CLI/parsing errors with context.
These branches return raw err, which drops call-site context. Wrap each return so upstream failures remain diagnosable.
💡 Suggested patch
_, err = cmd.Output()
if err != nil {
slog.Debug(fmt.Sprintf("Failed to run `heroku login`: %v", err))
- return err
+ return fmt.Errorf("failed to run `heroku login`: %w", err)
}
@@
err = authenticateHerokuCLI()
if err != nil {
slog.Debug(fmt.Sprintf("Failed to authenticate Heroku CLI: %v", err))
- return "", err
+ return "", fmt.Errorf("failed to authenticate Heroku CLI: %w", err)
}
@@
output, err := cmd.Output()
if err != nil {
slog.Debug(fmt.Sprintf("Failed to run `heroku authorizations:create`: %v", err))
- return "", err
+ return "", fmt.Errorf("failed to run `heroku authorizations:create`: %w", err)
}
@@
err = json.Unmarshal(output, &result)
if err != nil {
slog.Debug(fmt.Sprintf("Failed to parse Heroku CLI output: %v", err))
- return "", err
+ return "", fmt.Errorf("failed to parse Heroku CLI output: %w", err)
}As per coding guidelines, "Wrap errors with context using fmt.Errorf with %w verb".
Also applies to: 350-354, 359-362, 371-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/migrate/heroku.go` around lines 329 - 331, The code returns raw err
in multiple branches (e.g., after slog.Debug(fmt.Sprintf("Failed to run `heroku
login`: %v", err)) and similar blocks at the noted ranges), losing call-site
context; change each plain return err to return fmt.Errorf("failed to run heroku
login: %w", err) (or an equivalent contextual message matching the operation in
that branch), and ensure you import fmt if not present; apply the same wrapping
pattern to the other branches mentioned (lines 350-354, 359-362, 371-375) so all
CLI/parsing errors are wrapped with context using the %w verb.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/pkg/migrate/migrate.go (1)
78-78:⚠️ Potential issue | 🟠 MajorRemove pre-sanitization debug logging of full Heroku app data.
Line 78 logs
applicationInfobefore redaction, which can expose raw config vars/secrets in debug output.🔒 Proposed fix
- slog.Debug("Application info", "applicationInfo", applicationInfo) + slog.Debug("Collected Heroku application info", "sourceApp", sourceApp)#!/bin/bash # Verify the raw log happens before sanitization. rg -n 'slog\.Debug\("Application info"|sanitizeHerokuApplicationInfo\(' src/pkg/migrate/migrate.go nl -ba src/pkg/migrate/migrate.go | sed -n '72,88p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/migrate/migrate.go` at line 78, Remove the pre-sanitization debug log that prints raw Heroku app data: delete or replace the slog.Debug("Application info", "applicationInfo", applicationInfo) call in migrate.go so that sensitive config vars are not logged, and ensure any remaining logging uses the sanitized output from sanitizeHerokuApplicationInfo(applicationInfo) (or logs only non-sensitive fields) instead.src/pkg/cli/client/byoc/do/byoc.go (1)
113-113:⚠️ Potential issue | 🟡 MinorUse structured attributes instead of
fmt.Sprintin debug logs.Line 113 and Line 430 still flatten values into a single string. This loses structured fields and makes filtering harder.
♻️ Proposed fix
- slog.Debug(fmt.Sprint("s3.GetObject:", err)) + slog.DebugContext(ctx, "s3.GetObject no such key", "error", err) - slog.Debug(fmt.Sprint("Fetching app and deployment ID for app", appPlatform.CdName)) + slog.DebugContext(ctx, "Fetching app and deployment ID", "app", appPlatform.CdName)As per coding guidelines:
Use log/slog for structured debug logging.Also applies to: 430-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/do/byoc.go` at line 113, Replace the flattened fmt.Sprint call passed to slog.Debug with structured slog attributes: locate the slog.Debug call(s) that currently use fmt.Sprint("s3.GetObject:", err) (and the duplicate occurrence around line 430) and change them to call slog.Debug with a short message and structured fields (for example include operation/op="s3.GetObject" and the error as an attribute using slog.Any or slog.String/Value), so the log retains structured key/value pairs instead of a single concatenated string.src/cmd/cli/command/mcp.go (1)
63-68:⚠️ Potential issue | 🟠 MajorRoute MCP startup/shutdown messages through
pkg/term.These are user-facing CLI messages, but
fmt.Printlnbypasses the terminal abstraction and the mocked term buffer this PR is trying to keep aligned.Suggested change
- fmt.Println("Starting Defang MCP server") + term.Println("Starting Defang MCP server") if err := server.ServeStdio(s); err != nil { return err } - fmt.Println("Server shutdown") + term.Println("Server shutdown")As per coding guidelines,
src/cmd/cli/**/*.go: Use pkg/term for user-facing output with support for color and spinners.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/mcp.go` around lines 63 - 68, Replace the direct fmt.Println calls around server.ServeStdio(s) with the terminal abstraction in pkg/term so CLI output goes through the mocked term buffer; specifically, change the "Starting Defang MCP server" and "Server shutdown" prints used near server.ServeStdio(s) to the appropriate pkg/term output function (e.g., term.Println/term.Info/term.Infof as used elsewhere in the codebase), and update imports to add "pkg/term" and remove "fmt" if it becomes unused.src/cmd/cli/command/compose.go (1)
41-41:⚠️ Potential issue | 🟠 MajorKeep these user-facing compose messages on
pkg/term.These writes bypass the CLI terminal abstraction, so colors/spinners/mock-term capture stop being consistent and this output gets split away from the rest of the command UX.
Suggested change
- fmt.Println(" -", SERVICE_PORTAL_URL+"/"+serviceInfo.Service.Name) + term.Println(" -", SERVICE_PORTAL_URL+"/"+serviceInfo.Service.Name)- fmt.Println("This project was previously deployed to the following locations:") + term.Println("This project was previously deployed to the following locations:")- fmt.Println("Consider creating a stack to manage your deployments.") + term.Println("Consider creating a stack to manage your deployments.")- fmt.Printf("Showing logs%s; press Ctrl+C to stop:\n", rangeStr) + term.Printf("Showing logs%s; press Ctrl+C to stop:\n", rangeStr)As per coding guidelines,
src/cmd/cli/**/*.go: Use pkg/term for user-facing output with support for color and spinners.Also applies to: 259-259, 287-288, 680-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/compose.go` at line 41, The print to stdout using fmt.Println for the service URL bypasses the CLI terminal abstraction; replace the call that prints SERVICE_PORTAL_URL+"/"+serviceInfo.Service.Name in compose.go with the pkg/term user-facing output API (e.g., use the term package's Println/Printf or term.Out writer) so colors, spinners and mock-terminal capture remain consistent; update the other occurrences noted (the other fmt.Println/Printfs at the referenced locations) to use the same pkg/term functions and ensure the message content remains identical.
🧹 Nitpick comments (12)
src/pkg/cli/compose/validation.go (2)
404-417: Consider structured attributes for port validation warnings.These warnings embed
port.Targetand other values infmt.Sprintf. Using structured attributes would improve log parsing and filtering.♻️ Example refactor
- slog.Warn(fmt.Sprintf("port %d: 'published' range start should be an integer; ignoring 'published: %v'", port.Target, portRange[0])) + slog.Warn("'published' range start should be an integer; ignoring", "port", port.Target, "published", portRange[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/compose/validation.go` around lines 404 - 417, Replace the fmt.Sprintf-based warn calls with structured slog attributes so logs are parseable: locate the port validation block that uses slog.Warn(fmt.Sprintf(...)) (references: variables port, portRange, start, end and the slog.Warn calls) and change each call to slog.Warn with a short message string and key/value pairs for context (e.g. "target": port.Target, "published": port.Published, "rangeStart": portRange[0] or start, "rangeEnd": portRange[1] or end) so each branch (invalid start, invalid end, start> end, target out of range, and mismatch single value) emits structured attributes instead of formatting them into the message.
125-129: Consider structured attributes for warnings.Same pattern issue as in
fixup.go- theslog.Warncalls embed the service name infmt.Sprintfrather than using structured attributes.♻️ Suggested refactor
- slog.Warn(fmt.Sprintf("service %q: unsupported compose directive: volumes", svccfg.Name)) // TODO: add support for volumes + slog.Warn("unsupported compose directive: volumes", "service", svccfg.Name) // TODO: add support for volumesThis applies to other
slog.Warncalls throughout this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/compose/validation.go` around lines 125 - 129, Replace unstructured warning messages that use fmt.Sprintf with structured slog attributes: where you currently call slog.Warn(fmt.Sprintf("service %q: unsupported compose directive: volumes", svccfg.Name)) (and the volumes_from variant and other slog.Warn calls in this file), change to call slog.Warn with a plain message and include the service name as a structured attribute (e.g., slog.String("service", svccfg.Name)) and any other context as attributes; update all occurrences in src/pkg/cli/compose/validation.go that use fmt.Sprintf for service names (references: the slog.Warn calls and the svccfg.Name / svccfg.VolumesFrom checks) so logs are emitted with structured key/value attributes instead of interpolated strings.src/pkg/cli/compose/fixup.go (1)
86-86: Consider using structured attributes instead offmt.Sprintf.The current pattern embeds the service name in the message string, losing the benefits of structured logging. The codebase convention (see
src/pkg/cli/subscribe.goandsrc/pkg/track/track.go) is to pass data as separate attributes.♻️ Suggested refactor
- slog.WarnContext(ctx, fmt.Sprintf("service %q: using published image instead of rebuilding; pass --build to build and publish a new image", svccfg.Name)) + slog.WarnContext(ctx, "using published image instead of rebuilding; pass --build to build and publish a new image", "service", svccfg.Name)This pattern applies to other
slog.Warn/slog.WarnContextcalls in this file (lines 158, 165, 179, 207, 211, 217, 263, 287, 325, 451, 545, 550). As per coding guidelines: "Use log/slog for structured debug logging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/compose/fixup.go` at line 86, Replace formatted string logging with structured slog calls: for each occurrence of slog.WarnContext that currently uses fmt.Sprintf (e.g., the call emitting "service %q: using published image..." in fixup.go and the other occurrences in this file), pass a static message string and include the service name as a separate attribute (for example use key "service" and value svccfg.Name) instead of embedding it via fmt.Sprintf; update all listed sites (the call at the shown line plus the calls around the identifiers handling rebuild/publish logic) to follow the same pattern so structured attributes are logged consistently.src/pkg/cli/client/byoc/aws/stream.go (1)
93-93: Use structured fields instead of string concatenation inslog.Debug.At line 93, concatenating the log stream name into the message prevents structured querying. Prefer a static message with attributes instead.
Proposed change
- slog.Debug("unrecognized log stream format: " + *first.LogStreamName) + slog.Debug("unrecognized log stream format", "log_stream_name", *first.LogStreamName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/stream.go` at line 93, Replace the string-concatenation call to slog.Debug so the log message is static and the log stream name is passed as a structured attribute; locate the slog.Debug call that references first.LogStreamName in stream.go and change it to use slog.Debug with a static message and a slog.String (or slog.StringValue-style) attribute such as slog.Debug("unrecognized log stream format", slog.String("log_stream", *first.LogStreamName)) — ensure you handle the pointer safely if it can be nil before dereferencing.src/pkg/cli/client/caniuse.go (1)
78-87: Prefer structured fields inslog.Warn/slog.Infoinstead offmt.Sprintf.These calls flatten context into message strings. Passing attrs keeps logs queryable and avoids extra formatting work.
Suggested refactor
import ( "context" "errors" - "fmt" "log/slog" "os" @@ if forcedReason != "" { slog.Debug("Using version from fabric (forced)", "label", label, "version", fromFabric) - slog.Warn(fmt.Sprintf("Overriding %s: %s", label, forcedReason)) + slog.Warn("Overriding version", "label", label, "reason", forcedReason) return fromFabric } if allowUpgrade { slog.Debug("Using latest version from fabric", "label", label, "version", fromFabric) - slog.Info(fmt.Sprintf("Upgrading %s to latest", label)) + slog.Info("Upgrading to latest", "label", label, "from", previous, "to", fromFabric) return fromFabric } slog.Debug("Using previous version", "label", label, "version", previous) - slog.Warn(fmt.Sprintf("A newer %s is available; using previously deployed version. To upgrade, re-run with --allow-upgrade or set DEFANG_ALLOW_UPGRADE=1", label)) + slog.Warn("Newer version available; using previous deployment", "label", label, "current", previous, "available", fromFabric, "hint", "re-run with --allow-upgrade or set DEFANG_ALLOW_UPGRADE=1") return previous }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/caniuse.go` around lines 78 - 87, Replace formatted messages that use fmt.Sprintf in slog.Warn/slog.Info with structured logging using slog's key/value attributes: for the override case use slog.Warn with a short message like "Overriding version" and include "label" and "reason" attributes (referencing label and forcedReason); for the upgrade info replace slog.Info(fmt.Sprintf("Upgrading %s to latest", label)) with slog.Info("Upgrading to latest", "label", label) and keep the existing slog.Debug calls as-is; for the final warning replace the long fmt.Sprintf message with slog.Warn("Newer version available", "label", label, "note", "re-run with --allow-upgrade or set DEFANG_ALLOW_UPGRADE=1") while still calling return previous afterward. Ensure you update the calls in the same function where allowUpgrade, fromFabric, previous, label, and forcedReason are used.src/pkg/clouds/gcp/iam.go (1)
48-48: Refactorslogcalls to use structured attributes instead of string concatenation orfmt.Sprintf.The log lines at lines 48, 66, 111, 127, 195, 199, 272, 279, and 351 embed dynamic values into message text, which reduces the usefulness of structured logging for filtering, searching, and analysis.
♻️ Suggested refactor pattern
- slog.InfoContext(ctx, "Updating role "+roleId) + slog.InfoContext(ctx, "Updating role", "role_id", roleId) - slog.InfoContext(ctx, fmt.Sprintf("Updating IAM policy for principal %s on bucket %s", principal, bucketName)) + slog.InfoContext(ctx, "Updating IAM policy for bucket", "principal", principal, "bucket", bucketName) - slog.InfoContext(ctx, fmt.Sprintf("Failed to set IAM policy, will retry in %v: %v\n", retryInterval, err)) + slog.WarnContext(ctx, "Failed to set IAM policy; retrying", "retry_interval", retryInterval, "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/gcp/iam.go` at line 48, Replace string-concatenated slog calls with structured attributes: for each use of slog.InfoContext (e.g., the call that currently does slog.InfoContext(ctx, "Updating role "+roleId)), stop embedding dynamic values in the message and instead pass a constant message and key/value attributes (use slog.String or the appropriate slog attribute constructors) for roleId and other dynamic vars; update the same pattern at the other sites noted (lines with roleId and similar concatenations) so logs become like: slog.InfoContext(ctx, "Updating role", slog.String("role_id", roleId)) — apply this refactor for all slog.InfoContext/Warning/Error calls that currently use string concatenation or fmt.Sprintf.src/pkg/migrate/migrate.go (1)
71-71: Use structured slog fields instead offmt.Sprintffor variable data.Line 71 should keep a stable message and pass
sourceAppas a structured attribute.Suggested refactor
- slog.InfoContext(ctx, fmt.Sprintf("Collecting information about %q...", sourceApp)) + slog.InfoContext(ctx, "Collecting information about source application...", "sourceApp", sourceApp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/migrate/migrate.go` at line 71, Replace the formatted message call to slog.InfoContext so the log message is stable and the variable is passed as a structured field: locate the slog.InfoContext(ctx, fmt.Sprintf(...)) call and change it to a stable message (e.g., "Collecting information about app") and pass sourceApp as a structured attribute using the slog key/value form (reference the slog.InfoContext call and the sourceApp variable).src/pkg/cli/client/byoc/do/byoc.go (1)
459-459: PreferDebugContextwherectxis already available.This is optional, but using
slog.DebugContext(ctx, ...)at Line 459 and Line 703 improves consistency with surrounding context-aware logging.♻️ Suggested consistency refactor
- slog.Debug("Deployment phase", "phase", deploymentInfo.GetPhase()) + slog.DebugContext(ctx, "Deployment phase", "phase", deploymentInfo.GetPhase()) - slog.Debug("Creating new registry") + slog.DebugContext(ctx, "Creating new registry")Also applies to: 703-703
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/do/byoc.go` at line 459, Replace the context-less slog.Debug calls with context-aware logging: use slog.DebugContext(ctx, ...) where the current calls are slog.Debug("Deployment phase", "phase", deploymentInfo.GetPhase()) (and the similar call around the other occurrence at Line 703); ensure you pass the existing ctx variable as the first argument and keep the same message and key/value pairs so the call becomes DebugContext(ctx, "Deployment phase", "phase", deploymentInfo.GetPhase()) in both places.src/pkg/clouds/aws/login.go (1)
89-89: Avoidfmt.Sprintfinsideslog.WarnContext; log fields as structured attributes instead.These warnings currently flatten useful context into message strings. Keep messages static and attach dynamic data (
error,profile,redirectURL, etc.) as structured attrs.Proposed refactor
- slog.WarnContext(ctx, fmt.Sprintf("failed to persist refreshed AWS OAuth token: %v", err)) + slog.WarnContext(ctx, "failed to persist refreshed AWS OAuth token", "error", err) - slog.WarnContext(ctx, fmt.Sprintf("failed to save AWS OAuth token: %v", err)) + slog.WarnContext(ctx, "failed to save AWS OAuth token", "error", err) - slog.WarnContext(ctx, fmt.Sprintf("Cannot use login credentials: %v, please try again.", err)) + slog.WarnContext(ctx, "cannot use login credentials, please try again", "error", err) - slog.WarnContext(ctx, fmt.Sprintf("failed to get AWS_PROFILE role ARN: %v", err)) + slog.WarnContext(ctx, "failed to get AWS_PROFILE role ARN", "error", err) - slog.WarnContext(ctx, fmt.Sprintf("failed to compare token identity with AWS_PROFILE role: %v", err)) + slog.WarnContext(ctx, "failed to compare token identity with AWS_PROFILE role", "error", err) - slog.WarnContext(ctx, fmt.Sprintf("login successful for AWS account %v which is same as the account specified by stack aws profile %q, assume its valid", *identity.Account, profile)) + slog.WarnContext(ctx, "login successful for same AWS account as stack profile; assuming token is valid", "account", *identity.Account, "profile", profile) - slog.WarnContext(ctx, fmt.Sprintf("failed to parse redirect URL %q, assume port 8080: %v", redirectURL, err)) + slog.WarnContext(ctx, "failed to parse redirect URL, assuming port 8080", "redirectURL", redirectURL, "error", err)Lines 89, 183, 191, 260, 266, 292, 342.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/login.go` at line 89, Replace fmt.Sprintf usage inside slog.WarnContext calls with structured logging attributes: keep the message static (e.g., "failed to persist refreshed AWS OAuth token") and pass dynamic values as keyed attrs using slog.String / slog.Any / slog.Error (e.g., error, profile, redirectURL) instead of interpolating them into the message. Update each slog.WarnContext invocation in src/pkg/clouds/aws/login.go (the ones that currently call fmt.Sprintf) to use the static message and attach the relevant variables (err, profile, redirectURL, etc.) as structured attributes so logs remain machine-parseable and consistent.src/pkg/auth/auth.go (3)
115-120: Considerslog.DebugContextwhenctxis already available.
PollandExchangeCodeForTokenboth have a context in scope; usingslog.DebugContextcan preserve context-aware metadata for handlers.Suggested change
- slog.Debug("poll timed out, retrying...") + slog.DebugContext(ctx, "poll timed out, retrying...") @@ - slog.Debug("received server error, retrying", "status", unexpectedError.Status, "retryDelay", retryDelay) + slog.DebugContext(ctx, "received server error, retrying", "status", unexpectedError.Status, "retryDelay", retryDelay) @@ - slog.Debug("Generating access token", "scopes", scopes) + slog.DebugContext(ctx, "Generating access token", "scopes", scopes)As per coding guidelines, "Use log/slog for structured debug logging".
Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/auth/auth.go` around lines 115 - 120, Replace non-contextual slog.Debug calls with slog.DebugContext using the available ctx in functions Poll and ExchangeCodeForToken so structured logs carry context metadata; specifically, change the slog.Debug("poll timed out, retrying...") and the slog.Debug("received server error, retrying", "status", unexpectedError.Status, "retryDelay", retryDelay) to use slog.DebugContext(ctx, ...) (and the similar occurrence around line 165) so the log includes ctx while preserving the same message and key/value pairs.
61-61: Usetermfor this prompt to keep terminal output consistent.Line 61 writes directly to stdout via
fmt.Println, while adjacent auth-flow output usesterm. Keeping this line ontermimproves consistency and test/output capture behavior.Suggested change
- fmt.Println("Please visit the following URL to log in: (Right click the URL or press ENTER to open browser)") + term.Print("Please visit the following URL to log in: (Right click the URL or press ENTER to open browser)\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/auth/auth.go` at line 61, Replace the direct stdout call fmt.Println("Please visit the following URL...") with the shared terminal output helper (term) used elsewhere in the auth flow: call the equivalent term method (e.g., term.Println or term.Printf) with the same message so the prompt is written via term instead of fmt.Println, preserving the text and ensuring it uses the same writer/formatting as other auth output.
76-82: Use structured error attributes instead offmt.Sprintfinslog.ErrorContextcalls.Lines 76 and 82 interpolate the error into the message string. Per logging best practices, pass the error as a structured attribute (
"error", err) so handlers can parse, filter, and format errors reliably.Suggested change
- slog.ErrorContext(ctx, fmt.Sprintf("failed to poll for auth code: %v", err)) + slog.ErrorContext(ctx, "failed to poll for auth code", "error", err) return } @@ - slog.ErrorContext(ctx, fmt.Sprintf("failed to exchange code for token: %v", err)) + slog.ErrorContext(ctx, "failed to exchange code for token", "error", err) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/auth/auth.go` around lines 76 - 82, Replace the fmt.Sprintf-based interpolation in the two slog.ErrorContext calls with structured attributes: call slog.ErrorContext(ctx, "failed to poll for auth code", "error", err) for the first failure and slog.ErrorContext(ctx, "failed to exchange code for token", "error", err) for the ExchangeCodeForToken error; locate the slog.ErrorContext calls near the ExchangeCodeForToken invocation and the auth-code polling logic and pass the error as the "error" attribute (use slog.Any or the plain value) instead of embedding it in the message string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/compose.go`:
- Line 119: The slog calls currently concatenate messages with fmt.Sprint (e.g.,
slog.Debug(fmt.Sprint("Failed to create stack:", err))) which removes the space
and bypasses structured logging; replace each fmt.Sprint-backed slog invocation
in this file (examples: the slog.Debug in compose.go that uses "Failed to create
stack:", and the similar calls at the other listed locations) with structured
slog calls that pass attributes instead (e.g., use slog.Debug("Failed to create
stack", "err", err) or slog.Debug("message", slog.String("key", val)) so the
message and fields are separate); update all occurrences at the specified spots
(lines referenced in the review: 119, 164, 172, 314, 326, 335, 347, 447, 524,
534) to follow this pattern and remove fmt.Sprint usage.
In `@src/cmd/cli/command/stack.go`:
- Around line 139-140: Replace the direct slog.InfoContext CLI messages in
stack.go (the calls to slog.InfoContext(ctx, "No Defang stacks found in the
current directory.") and the similar messages at the other occurrences around
lines 177-178) with the pkg/term user-facing output API (e.g., term.Info or
term.Println) so output goes through the terminal helper; make the term call's
error (if any) propagate by returning it instead of returning nil. Ensure you
import pkg/term and remove/replace slog.InfoContext usages for these user-facing
messages.
In `@src/pkg/cli/client/byoc/aws/list.go`:
- Around line 5-8: The call using slog.Debug(fmt.Sprint(...)) should be
converted to structured logging: remove the fmt import from the import block,
replace the fmt.Sprint usage with slog.Debug and pass key/value pairs (e.g.,
"msg", "listing", "count", value, "region", region) so fields are structured;
update the specific slog.Debug call in src/pkg/cli/client/byoc/aws/list.go
(replace the fmt.Sprint(...) argument) to use slog.Debug context-style arguments
and ensure fmt is no longer imported.
In `@src/pkg/cli/new.go`:
- Line 104: Replace direct fmt.Println and fmt.Sprintf usages with structured
slog calls: change the fmt.Println(" -", base) to an slog.Info (or slog.Debug
depending on level) call that logs a descriptive message and a structured field
for the base (e.g., message "available base" with key "base" and value base),
and update the slog call that currently embeds fmt.Sprintf to remove the sprintf
and pass the pieces as structured fields instead (use a clear message string and
add keys for any dynamic values). Locate the occurrences by the literal
fmt.Println(" -", base) and the slog call that currently contains fmt.Sprintf,
and replace them so all logging uses slog with structured fields rather than
formatting into a single string.
In `@src/pkg/clouds/aws/login.go`:
- Line 369: Replace the direct call to fmt.Println that prints the AWS login URL
with the terminal abstraction (term) so output goes through the mocked terminal;
specifically, in the same scope where you later use the term variable, swap
fmt.Println("Please visit the following URL to log in to AWS: (Right click the
URL or press ENTER to open browser)") for the equivalent term method (e.g.,
term.Println(...) or term.Printf(...)) so the prompt uses term instead of
fmt.Println and keeps tests/mocks consistent.
---
Duplicate comments:
In `@src/cmd/cli/command/compose.go`:
- Line 41: The print to stdout using fmt.Println for the service URL bypasses
the CLI terminal abstraction; replace the call that prints
SERVICE_PORTAL_URL+"/"+serviceInfo.Service.Name in compose.go with the pkg/term
user-facing output API (e.g., use the term package's Println/Printf or term.Out
writer) so colors, spinners and mock-terminal capture remain consistent; update
the other occurrences noted (the other fmt.Println/Printfs at the referenced
locations) to use the same pkg/term functions and ensure the message content
remains identical.
In `@src/cmd/cli/command/mcp.go`:
- Around line 63-68: Replace the direct fmt.Println calls around
server.ServeStdio(s) with the terminal abstraction in pkg/term so CLI output
goes through the mocked term buffer; specifically, change the "Starting Defang
MCP server" and "Server shutdown" prints used near server.ServeStdio(s) to the
appropriate pkg/term output function (e.g., term.Println/term.Info/term.Infof as
used elsewhere in the codebase), and update imports to add "pkg/term" and remove
"fmt" if it becomes unused.
In `@src/pkg/cli/client/byoc/do/byoc.go`:
- Line 113: Replace the flattened fmt.Sprint call passed to slog.Debug with
structured slog attributes: locate the slog.Debug call(s) that currently use
fmt.Sprint("s3.GetObject:", err) (and the duplicate occurrence around line 430)
and change them to call slog.Debug with a short message and structured fields
(for example include operation/op="s3.GetObject" and the error as an attribute
using slog.Any or slog.String/Value), so the log retains structured key/value
pairs instead of a single concatenated string.
In `@src/pkg/migrate/migrate.go`:
- Line 78: Remove the pre-sanitization debug log that prints raw Heroku app
data: delete or replace the slog.Debug("Application info", "applicationInfo",
applicationInfo) call in migrate.go so that sensitive config vars are not
logged, and ensure any remaining logging uses the sanitized output from
sanitizeHerokuApplicationInfo(applicationInfo) (or logs only non-sensitive
fields) instead.
---
Nitpick comments:
In `@src/pkg/auth/auth.go`:
- Around line 115-120: Replace non-contextual slog.Debug calls with
slog.DebugContext using the available ctx in functions Poll and
ExchangeCodeForToken so structured logs carry context metadata; specifically,
change the slog.Debug("poll timed out, retrying...") and the
slog.Debug("received server error, retrying", "status", unexpectedError.Status,
"retryDelay", retryDelay) to use slog.DebugContext(ctx, ...) (and the similar
occurrence around line 165) so the log includes ctx while preserving the same
message and key/value pairs.
- Line 61: Replace the direct stdout call fmt.Println("Please visit the
following URL...") with the shared terminal output helper (term) used elsewhere
in the auth flow: call the equivalent term method (e.g., term.Println or
term.Printf) with the same message so the prompt is written via term instead of
fmt.Println, preserving the text and ensuring it uses the same writer/formatting
as other auth output.
- Around line 76-82: Replace the fmt.Sprintf-based interpolation in the two
slog.ErrorContext calls with structured attributes: call slog.ErrorContext(ctx,
"failed to poll for auth code", "error", err) for the first failure and
slog.ErrorContext(ctx, "failed to exchange code for token", "error", err) for
the ExchangeCodeForToken error; locate the slog.ErrorContext calls near the
ExchangeCodeForToken invocation and the auth-code polling logic and pass the
error as the "error" attribute (use slog.Any or the plain value) instead of
embedding it in the message string.
In `@src/pkg/cli/client/byoc/aws/stream.go`:
- Line 93: Replace the string-concatenation call to slog.Debug so the log
message is static and the log stream name is passed as a structured attribute;
locate the slog.Debug call that references first.LogStreamName in stream.go and
change it to use slog.Debug with a static message and a slog.String (or
slog.StringValue-style) attribute such as slog.Debug("unrecognized log stream
format", slog.String("log_stream", *first.LogStreamName)) — ensure you handle
the pointer safely if it can be nil before dereferencing.
In `@src/pkg/cli/client/byoc/do/byoc.go`:
- Line 459: Replace the context-less slog.Debug calls with context-aware
logging: use slog.DebugContext(ctx, ...) where the current calls are
slog.Debug("Deployment phase", "phase", deploymentInfo.GetPhase()) (and the
similar call around the other occurrence at Line 703); ensure you pass the
existing ctx variable as the first argument and keep the same message and
key/value pairs so the call becomes DebugContext(ctx, "Deployment phase",
"phase", deploymentInfo.GetPhase()) in both places.
In `@src/pkg/cli/client/caniuse.go`:
- Around line 78-87: Replace formatted messages that use fmt.Sprintf in
slog.Warn/slog.Info with structured logging using slog's key/value attributes:
for the override case use slog.Warn with a short message like "Overriding
version" and include "label" and "reason" attributes (referencing label and
forcedReason); for the upgrade info replace slog.Info(fmt.Sprintf("Upgrading %s
to latest", label)) with slog.Info("Upgrading to latest", "label", label) and
keep the existing slog.Debug calls as-is; for the final warning replace the long
fmt.Sprintf message with slog.Warn("Newer version available", "label", label,
"note", "re-run with --allow-upgrade or set DEFANG_ALLOW_UPGRADE=1") while still
calling return previous afterward. Ensure you update the calls in the same
function where allowUpgrade, fromFabric, previous, label, and forcedReason are
used.
In `@src/pkg/cli/compose/fixup.go`:
- Line 86: Replace formatted string logging with structured slog calls: for each
occurrence of slog.WarnContext that currently uses fmt.Sprintf (e.g., the call
emitting "service %q: using published image..." in fixup.go and the other
occurrences in this file), pass a static message string and include the service
name as a separate attribute (for example use key "service" and value
svccfg.Name) instead of embedding it via fmt.Sprintf; update all listed sites
(the call at the shown line plus the calls around the identifiers handling
rebuild/publish logic) to follow the same pattern so structured attributes are
logged consistently.
In `@src/pkg/cli/compose/validation.go`:
- Around line 404-417: Replace the fmt.Sprintf-based warn calls with structured
slog attributes so logs are parseable: locate the port validation block that
uses slog.Warn(fmt.Sprintf(...)) (references: variables port, portRange, start,
end and the slog.Warn calls) and change each call to slog.Warn with a short
message string and key/value pairs for context (e.g. "target": port.Target,
"published": port.Published, "rangeStart": portRange[0] or start, "rangeEnd":
portRange[1] or end) so each branch (invalid start, invalid end, start> end,
target out of range, and mismatch single value) emits structured attributes
instead of formatting them into the message.
- Around line 125-129: Replace unstructured warning messages that use
fmt.Sprintf with structured slog attributes: where you currently call
slog.Warn(fmt.Sprintf("service %q: unsupported compose directive: volumes",
svccfg.Name)) (and the volumes_from variant and other slog.Warn calls in this
file), change to call slog.Warn with a plain message and include the service
name as a structured attribute (e.g., slog.String("service", svccfg.Name)) and
any other context as attributes; update all occurrences in
src/pkg/cli/compose/validation.go that use fmt.Sprintf for service names
(references: the slog.Warn calls and the svccfg.Name / svccfg.VolumesFrom
checks) so logs are emitted with structured key/value attributes instead of
interpolated strings.
In `@src/pkg/clouds/aws/login.go`:
- Line 89: Replace fmt.Sprintf usage inside slog.WarnContext calls with
structured logging attributes: keep the message static (e.g., "failed to persist
refreshed AWS OAuth token") and pass dynamic values as keyed attrs using
slog.String / slog.Any / slog.Error (e.g., error, profile, redirectURL) instead
of interpolating them into the message. Update each slog.WarnContext invocation
in src/pkg/clouds/aws/login.go (the ones that currently call fmt.Sprintf) to use
the static message and attach the relevant variables (err, profile, redirectURL,
etc.) as structured attributes so logs remain machine-parseable and consistent.
In `@src/pkg/clouds/gcp/iam.go`:
- Line 48: Replace string-concatenated slog calls with structured attributes:
for each use of slog.InfoContext (e.g., the call that currently does
slog.InfoContext(ctx, "Updating role "+roleId)), stop embedding dynamic values
in the message and instead pass a constant message and key/value attributes (use
slog.String or the appropriate slog attribute constructors) for roleId and other
dynamic vars; update the same pattern at the other sites noted (lines with
roleId and similar concatenations) so logs become like: slog.InfoContext(ctx,
"Updating role", slog.String("role_id", roleId)) — apply this refactor for all
slog.InfoContext/Warning/Error calls that currently use string concatenation or
fmt.Sprintf.
In `@src/pkg/migrate/migrate.go`:
- Line 71: Replace the formatted message call to slog.InfoContext so the log
message is stable and the variable is passed as a structured field: locate the
slog.InfoContext(ctx, fmt.Sprintf(...)) call and change it to a stable message
(e.g., "Collecting information about app") and pass sourceApp as a structured
attribute using the slog key/value form (reference the slog.InfoContext call and
the sourceApp variable).
🪄 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: f8228745-0669-4198-804a-d24f03fb1208
📒 Files selected for processing (58)
src/cmd/cli/command/compose.gosrc/cmd/cli/command/estimate.gosrc/cmd/cli/command/generate.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/mcp.gosrc/cmd/cli/command/session.gosrc/cmd/cli/command/stack.gosrc/pkg/agent/common/common.gosrc/pkg/agent/generator.gosrc/pkg/agent/toolmanager.gosrc/pkg/agent/tools/deploy.gosrc/pkg/agent/tools/estimate.gosrc/pkg/agent/tools/services.gosrc/pkg/auth/auth.gosrc/pkg/cli/cd.gosrc/pkg/cli/cert.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/domain.gosrc/pkg/cli/client/byoc/aws/list.gosrc/pkg/cli/client/byoc/aws/stream.gosrc/pkg/cli/client/byoc/aws/subscribe.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/caniuse.gosrc/pkg/cli/client/cluster.gosrc/pkg/cli/compose/baseimage.gosrc/pkg/cli/compose/context.gosrc/pkg/cli/compose/dockerfile_validator.gosrc/pkg/cli/compose/fixup.gosrc/pkg/cli/compose/loader.gosrc/pkg/cli/compose/serviceNameReplacer.gosrc/pkg/cli/compose/validation.gosrc/pkg/cli/composeDown.gosrc/pkg/cli/configDelete.gosrc/pkg/cli/configList.gosrc/pkg/cli/configSet.gosrc/pkg/cli/connect.gosrc/pkg/cli/estimate.gosrc/pkg/cli/getServices.gosrc/pkg/cli/new.gosrc/pkg/cli/subscribe.gosrc/pkg/cli/tail.gosrc/pkg/cli/token.gosrc/pkg/cli/upgrade.gosrc/pkg/cli/waitForCdTaskExit.gosrc/pkg/clouds/aws/login.gosrc/pkg/clouds/do/appPlatform/setup.gosrc/pkg/clouds/gcp/api.gosrc/pkg/clouds/gcp/iam.gosrc/pkg/clouds/gcp/login.gosrc/pkg/clouds/gcp/storage.gosrc/pkg/github/version.gosrc/pkg/mcp/mcp_server.gosrc/pkg/mcp/utils.gosrc/pkg/migrate/heroku.gosrc/pkg/migrate/migrate.gosrc/pkg/track/track.go
✅ Files skipped from review due to trivial changes (18)
- src/pkg/cli/compose/baseimage.go
- src/pkg/agent/toolmanager.go
- src/pkg/agent/tools/estimate.go
- src/pkg/cli/getServices.go
- src/pkg/cli/configSet.go
- src/pkg/cli/compose/dockerfile_validator.go
- src/pkg/mcp/utils.go
- src/pkg/github/version.go
- src/cmd/cli/command/estimate.go
- src/pkg/cli/waitForCdTaskExit.go
- src/pkg/agent/generator.go
- src/pkg/track/track.go
- src/pkg/clouds/gcp/login.go
- src/cmd/cli/command/session.go
- src/pkg/cli/configDelete.go
- src/pkg/cli/client/byoc/aws/byoc.go
- src/pkg/mcp/mcp_server.go
- src/pkg/agent/tools/services.go
🚧 Files skipped from review as they are similar to previous changes (25)
- src/pkg/cli/client/byoc/aws/subscribe.go
- src/pkg/cli/upgrade.go
- src/pkg/cli/composeDown.go
- src/pkg/cli/token.go
- src/cmd/cli/command/globals.go
- src/pkg/clouds/do/appPlatform/setup.go
- src/pkg/cli/estimate.go
- src/pkg/cli/compose/loader.go
- src/pkg/clouds/gcp/storage.go
- src/pkg/cli/configList.go
- src/pkg/clouds/gcp/api.go
- src/cmd/cli/command/generate.go
- src/pkg/cli/client/byoc/gcp/stream.go
- src/pkg/cli/connect.go
- src/pkg/cli/compose/serviceNameReplacer.go
- src/pkg/cli/client/cluster.go
- src/pkg/cli/client/byoc/gcp/byoc.go
- src/pkg/cli/cert.go
- src/pkg/migrate/heroku.go
- src/pkg/cli/compose/context.go
- src/pkg/cli/client/byoc/aws/domain.go
- src/pkg/cli/subscribe.go
- src/pkg/agent/common/common.go
- src/pkg/cli/cd.go
- src/pkg/cli/tail.go
| }) | ||
| if err != nil { | ||
| term.Debug("Failed to create stack:", err) | ||
| slog.Debug(fmt.Sprint("Failed to create stack:", err)) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and its total line count
wc -l src/cmd/cli/command/compose.goRepository: DefangLabs/defang
Length of output: 95
🏁 Script executed:
# Now let's look at the specific lines mentioned
sed -n '115,125p' src/cmd/cli/command/compose.go | cat -nRepository: DefangLabs/defang
Length of output: 455
🏁 Script executed:
# Check lines around 164
sed -n '160,170p' src/cmd/cli/command/compose.go | cat -nRepository: DefangLabs/defang
Length of output: 696
🏁 Script executed:
# Check lines around 172
sed -n '168,178p' src/cmd/cli/command/compose.go | cat -nRepository: DefangLabs/defang
Length of output: 564
🏁 Script executed:
# Check all the other lines: 314, 326, 335, 347, 447, 524, 534
sed -n '310,320p' src/cmd/cli/command/compose.go | cat -n
sed -n '322,332p' src/cmd/cli/command/compose.go | cat -n
sed -n '331,341p' src/cmd/cli/command/compose.go | cat -n
sed -n '343,353p' src/cmd/cli/command/compose.go | cat -n
sed -n '443,453p' src/cmd/cli/command/compose.go | cat -n
sed -n '520,530p' src/cmd/cli/command/compose.go | cat -n
sed -n '530,540p' src/cmd/cli/command/compose.go | cat -nRepository: DefangLabs/defang
Length of output: 3489
🏁 Script executed:
# Let's test the fmt.Sprint behavior to confirm the space issue
python3 << 'EOF'
import subprocess
# Test Go behavior
test_go_code = '''package main
import (
"fmt"
"errors"
)
func main() {
err := errors.New("operation failed")
// Test case 1: fmt.Sprint with colon
result1 := fmt.Sprint("Failed to create stack:", err)
fmt.Println("Test 1 (fmt.Sprint):", result1)
// Test case 2: fmt.Sprintf with proper spacing
result2 := fmt.Sprintf("Failed to create stack: %v", err)
fmt.Println("Test 2 (fmt.Sprintf):", result2)
// Test case 3: Direct concatenation
result3 := "Failed to create stack: " + err.Error()
fmt.Println("Test 3 (concatenation):", result3)
}
'''
# Write the test file
with open('/tmp/test_fmt.go', 'w') as f:
f.write(test_go_code)
# Run the test
result = subprocess.run(['go', 'run', '/tmp/test_fmt.go'], capture_output=True, text=True)
print(result.stdout)
if result.stderr:
print("STDERR:", result.stderr)
EOFRepository: DefangLabs/defang
Length of output: 250
Several slog messages lose spaces because they are built with fmt.Sprint.
fmt.Sprint("prefix:", err) produces prefix:<err> without a space. Use structured logging with attributes instead:
-slog.Debug(fmt.Sprint("Failed to create stack:", err))
+slog.Debug("Failed to create stack", "err", err)-slog.InfoContext(ctx, fmt.Sprint("Tailing logs for", tailSource, "; press Ctrl+C to detach:"))
+slog.InfoContext(ctx, "Tailing logs; press Ctrl+C to detach", "source", tailSource)-slog.Info(fmt.Sprint("Deleted services, deployment ID", deployment))
+slog.Info("Deleted services", "deployment", deployment)Apply the same refactoring to all fmt.Sprint-backed slog calls in this file (lines 119, 164, 172, 314, 326, 335, 347, 447, 524, 534).
📝 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
| slog.Debug(fmt.Sprint("Failed to create stack:", err)) | |
| slog.Debug("Failed to create stack", "err", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/cli/command/compose.go` at line 119, The slog calls currently
concatenate messages with fmt.Sprint (e.g., slog.Debug(fmt.Sprint("Failed to
create stack:", err))) which removes the space and bypasses structured logging;
replace each fmt.Sprint-backed slog invocation in this file (examples: the
slog.Debug in compose.go that uses "Failed to create stack:", and the similar
calls at the other listed locations) with structured slog calls that pass
attributes instead (e.g., use slog.Debug("Failed to create stack", "err", err)
or slog.Debug("message", slog.String("key", val)) so the message and fields are
separate); update all occurrences at the specified spots (lines referenced in
the review: 119, 164, 172, 314, 326, 335, 347, 447, 524, 534) to follow this
pattern and remove fmt.Sprint usage.
Comment on lines
+139
to
+140
| slog.InfoContext(ctx, "No Defang stacks found in the current directory.") | ||
| return nil |
Contributor
There was a problem hiding this comment.
Keep user-facing CLI messages on pkg/term instead of slog.
These messages are direct CLI UX output; route them through term and return any write error from term calls.
Suggested patch
if len(stacks) == 0 {
- slog.InfoContext(ctx, "No Defang stacks found in the current directory.")
- return nil
+ return term.Infof("No Defang stacks found in the current directory.")
}
@@
- slog.InfoContext(ctx, fmt.Sprintf("Stack %q is now the default stack for project %q\n", name, projectName))
- return nil
+ return term.Infof("Stack %q is now the default stack for project %q", name, projectName)
},
}As per coding guidelines, "src/cmd/cli/**/*.go: Use pkg/term for user-facing output with support for color and spinners".
Also applies to: 177-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/cli/command/stack.go` around lines 139 - 140, Replace the direct
slog.InfoContext CLI messages in stack.go (the calls to slog.InfoContext(ctx,
"No Defang stacks found in the current directory.") and the similar messages at
the other occurrences around lines 177-178) with the pkg/term user-facing output
API (e.g., term.Info or term.Println) so output goes through the terminal
helper; make the term call's error (if any) propagate by returning it instead of
returning nil. Ensure you import pkg/term and remove/replace slog.InfoContext
usages for these user-facing messages.
Comment on lines
+5
to
+8
| "fmt" | ||
| "io" | ||
| "iter" | ||
| "log/slog" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no preformatted slog.Debug calls and no leftover fmt import in this file.
rg -nP 'slog\.Debug\(\s*fmt\.S(print|printf)\(' src/pkg/cli/client/byoc/aws/list.go
rg -nP '^\s*"fmt"' src/pkg/cli/client/byoc/aws/list.goRepository: DefangLabs/defang
Length of output: 137
Use structured slog.Debug with key-value pairs instead of preformatted fmt.Sprint message.
Line 65 uses slog.Debug(fmt.Sprint(...)) which bypasses structured logging and unnecessarily imports fmt. Replace with structured key-value attributes.
Proposed fix
import (
"context"
- "fmt"
"io"
"iter"
"log/slog"
"strings"
"sync"
@@
- slog.Debug(fmt.Sprint("Listing stacks in bucket:", bucketName))
+ slog.Debug("Listing stacks in bucket", "bucket", bucketName)📝 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
| "fmt" | |
| "io" | |
| "iter" | |
| "log/slog" | |
| "io" | |
| "iter" | |
| "log/slog" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/cli/client/byoc/aws/list.go` around lines 5 - 8, The call using
slog.Debug(fmt.Sprint(...)) should be converted to structured logging: remove
the fmt import from the import block, replace the fmt.Sprint usage with
slog.Debug and pass key/value pairs (e.g., "msg", "listing", "count", value,
"region", region) so fields are structured; update the specific slog.Debug call
in src/pkg/cli/client/byoc/aws/list.go (replace the fmt.Sprint(...) argument) to
use slog.Debug context-style arguments and ensure fmt is no longer imported.
| if base, ok := strings.CutPrefix(h.Name, prefix); ok && len(base) > 0 { | ||
| sampleFound = true | ||
| term.Println(" -", base) | ||
| fmt.Println(" -", base) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this file does not bypass slog or pre-format slog messages.
# Expected: no matches after applying the patch.
rg -n --type go 'fmt\.Println\(' src/pkg/cli/new.go
rg -n --type go 'slog\.(Debug|Info|Warn|Error)(Context)?\([^)]*fmt\.Sprintf\(' src/pkg/cli/new.goRepository: DefangLabs/defang
Length of output: 181
🏁 Script executed:
sed -n '95,125p' src/pkg/cli/new.go | cat -nRepository: DefangLabs/defang
Length of output: 1172
Replace fmt.Println and fmt.Sprintf in logging with slog structured calls.
At Line 104, fmt.Println bypasses the configured slog sink. At Line 117, fmt.Sprintf inside the slog call loses structured fields and prevents proper log formatting.
🔧 Suggested patch
- fmt.Println(" -", base)
+ slog.InfoContext(ctx, "Copying sample file", "file", base)
...
- slog.WarnContext(ctx, fmt.Sprintf("File already exists, skipping: %q", path))
+ slog.WarnContext(ctx, "File already exists, skipping", "path", path)Per coding guidelines: "**/*.go: Use log/slog for structured debug logging".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/cli/new.go` at line 104, Replace direct fmt.Println and fmt.Sprintf
usages with structured slog calls: change the fmt.Println(" -", base) to an
slog.Info (or slog.Debug depending on level) call that logs a descriptive
message and a structured field for the base (e.g., message "available base" with
key "base" and value base), and update the slog call that currently embeds
fmt.Sprintf to remove the sprintf and pass the pieces as structured fields
instead (use a clear message string and add keys for any dynamic values). Locate
the occurrences by the literal fmt.Println(" -", base) and the slog call that
currently contains fmt.Sprintf, and replace them so all logging uses slog with
structured fields rather than formatting into a single string.
| authURL := auth.GetAuthorizeUrl("aws", "cross", string(a.Region), state, pkce.Challenge) | ||
|
|
||
| term.Println("Please visit the following URL to log in to AWS: (Right click the URL or press ENTER to open browser)") | ||
| fmt.Println("Please visit the following URL to log in to AWS: (Right click the URL or press ENTER to open browser)") |
Contributor
There was a problem hiding this comment.
Use term for this prompt instead of fmt.Println to keep output routing consistent.
This line bypasses the terminal abstraction used immediately below and can escape mocked term buffers in tests.
Proposed fix
- fmt.Println("Please visit the following URL to log in to AWS: (Right click the URL or press ENTER to open browser)")
+ term.Printf("Please visit the following URL to log in to AWS: (Right click the URL or press ENTER to open browser)\n")📝 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
| fmt.Println("Please visit the following URL to log in to AWS: (Right click the URL or press ENTER to open browser)") | |
| term.Printf("Please visit the following URL to log in to AWS: (Right click the URL or press ENTER to open browser)\n") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/clouds/aws/login.go` at line 369, Replace the direct call to
fmt.Println that prints the AWS login URL with the terminal abstraction (term)
so output goes through the mocked terminal; specifically, in the same scope
where you later use the term variable, swap fmt.Println("Please visit the
following URL to log in to AWS: (Right click the URL or press ENTER to open
browser)") for the equivalent term method (e.g., term.Println(...) or
term.Printf(...)) so the prompt uses term instead of fmt.Println and keeps
tests/mocks consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrated term to slog. Tests that mock
term.DefaultTermnow need to also update the slog default logger to capture output for assertions.Key changes
slog.SetDefault(logs.NewTermLogger(term.DefaultTerm))after replacingterm.DefaultTermin 11 test files to ensure slog output goes to the mock term buffertermHandlerinpkg/logs/slog.goto fix data race inTailAndMonitorwhen multiple goroutines write concurrentlytail_test.goto handle variable number of trailing spaces when clearing the reconnection messageFiles modified (test files)
cmd/cli/command/compose_test.gocmd/cli/command/stack_test.go(MockTerm helper)pkg/cli/client/byoc/aws/byoc_test.gopkg/cli/compose/compose_test.gopkg/cli/compose/serviceNameReplacer_test.gopkg/cli/compose/validation_test.gopkg/cli/configList_test.gopkg/cli/configResolution_test.gopkg/cli/deploymentsList_test.gopkg/cli/getServices_test.gopkg/cli/tail_test.goNote
The mutex in
termHandlerfixes a pre-existing data race that was exposed by the slog migration. Two concurrent goroutines inTailAndMonitorwere writing to the same buffer without synchronization.Summary by CodeRabbit