Open
Conversation
carole-lavillonniere
changed the title
📝 WalkthroughWalkthroughThe PR refactors command registration from init-time global variables to constructor functions and centralizes environment configuration. Commands now receive configuration via parameters instead of relying on global state. The root command is created dynamically via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/api/client.go (1)
70-73: NormalizeapiEndpointin the constructor to avoid double-slash URLs.If callers provide a trailing slash, request URLs become
//v1/.... Trimming once at construction avoids that class of issues.Proposed patch
import ( "bytes" "context" "encoding/json" "fmt" "log" "net/http" + "strings" "time" "github.com/localstack/lstk/internal/version" ) @@ func NewPlatformClient(apiEndpoint string) *PlatformClient { return &PlatformClient{ - baseURL: apiEndpoint, + baseURL: strings.TrimRight(apiEndpoint, "/"), httpClient: &http.Client{Timeout: 30 * time.Second}, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 70 - 73, The constructor NewPlatformClient should normalize the apiEndpoint to avoid double-slash URLs; update NewPlatformClient to trim any trailing slash from the apiEndpoint before assigning it to PlatformClient.baseURL (e.g., use strings.TrimRight/TrimSuffix) so subsequent request path joins do not produce `//v1/...`; change the assignment in NewPlatformClient to set baseURL to the trimmed value and import the strings package if needed.cmd/logout.go (1)
32-32: Usecfg.WebAppURLinstead of a hard-coded empty URL inauth.New.Passing
""at Line 32 makes logout behavior depend onauth.Newnever requiring/validating that field. Supplyingcfg.WebAppURLkeeps constructor inputs consistent across auth flows and reduces hidden coupling.♻️ Proposed change
- a := auth.New(sink, platformClient, tokenStorage, cfg.AuthToken, "", false) + a := auth.New(sink, platformClient, tokenStorage, cfg.AuthToken, cfg.WebAppURL, false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/logout.go` at line 32, Replace the hard-coded empty string passed into auth.New with the configured web app URL to keep constructor inputs consistent: update the call to auth.New (currently a := auth.New(sink, platformClient, tokenStorage, cfg.AuthToken, "", false)) to pass cfg.WebAppURL instead of "" so logout uses the same WebApp URL as other auth flows and avoids hidden coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 25-37: The fmt.Fprintf calls that write errors to stderr after
runtime.NewDockerRuntime() and after runStart() must have their returned values
checked; locate the two fmt.Fprintf(...) usages in the Run function (surrounding
runtime.NewDockerRuntime and the runStart error handling that uses
output.IsSilent) and replace the ignored write with a checked write (capture the
returned n, err := fmt.Fprintf(...)) and handle any write error before calling
os.Exit(1) — e.g., if fmt.Fprintf returns an error, attempt a fallback write
(such as fmt.Fprintln(os.Stderr, "failed to write to stderr:", writeErr)) or log
the write failure so the process still exits with the intended status; ensure
both occurrences are updated so no write errors are ignored.
- Around line 63-64: The code calls env.Init() before config.Init(), causing cfg
to capture defaults instead of config-file overrides; update NewRootCmd/command
wiring so environment initialization runs after configuration is loaded—either
call config.Init() before env.Init() or move env.Init() into the command's
PreRunE (e.g., after initConfig runs) so runStart and any use of cfg reflect
config-file values; locate env.Init(), config.Init(), NewRootCmd, PreRunE, and
runStart in the diff and reorder or invoke env.Init() from PreRunE (after
initConfig) accordingly.
In `@cmd/stop.go`:
- Line 26: The call to fmt.Println(msg) drops its returned error; change it to
check and handle the error (e.g. if _, err := fmt.Println(msg); err != nil {
return err } or log the error and os.Exit(1) depending on the surrounding
function's error-handling pattern) so write failures (broken pipe, etc.) are not
ignored; update the statement that currently reads fmt.Println(msg) to perform
the error check and propagate or exit consistently with the caller.
- Around line 25-29: The stop command currently creates an onProgress func and
prints directly, then passes that callback into container.Stop; instead, remove
the UI callback from the domain call and route progress via the typed
internal/output event sinks at the cmd boundary: stop building an event sink (or
use the existing internal/output emitter) in the cmd/ stop handler,
subscribe/send progress events there, and change the call site so
container.Stop(ctx, rt) no longer accepts onProgress; update container.Stop (and
any downstream domain functions) to emit typed progress events via the existing
output/event interfaces rather than receiving a UI callback (references:
onProgress, container.Stop, internal/output event sinks).
In `@internal/auth/auth.go`:
- Around line 65-70: The logout path currently treats keyring deletion as a full
logout even when an injected token (a.authToken) remains set, causing a false
success report; in the blocks around token retrieval and keyring deletion
(references: a.tokenStorage.GetAuthToken, a.authToken, output.EmitNote), change
the control flow so that if a.authToken != "" you do NOT report a successful
logout — instead emit a clear warning/note that an injected
LOCALSTACK_AUTH_TOKEN remains active and return a non-success result (or an
explicit error/exit) so callers/users are informed that logout is incomplete;
apply the same fix to the other identical branch around lines 84-86.
---
Nitpick comments:
In `@cmd/logout.go`:
- Line 32: Replace the hard-coded empty string passed into auth.New with the
configured web app URL to keep constructor inputs consistent: update the call to
auth.New (currently a := auth.New(sink, platformClient, tokenStorage,
cfg.AuthToken, "", false)) to pass cfg.WebAppURL instead of "" so logout uses
the same WebApp URL as other auth flows and avoids hidden coupling.
In `@internal/api/client.go`:
- Around line 70-73: The constructor NewPlatformClient should normalize the
apiEndpoint to avoid double-slash URLs; update NewPlatformClient to trim any
trailing slash from the apiEndpoint before assigning it to
PlatformClient.baseURL (e.g., use strings.TrimRight/TrimSuffix) so subsequent
request path joins do not produce `//v1/...`; change the assignment in
NewPlatformClient to set baseURL to the trimmed value and import the strings
package if needed.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/config.gocmd/help_test.gocmd/login.gocmd/logout.gocmd/logs.gocmd/root.gocmd/start.gocmd/stop.gocmd/version.gointernal/api/client.gointernal/auth/auth.gointernal/auth/login.gointernal/auth/token_storage.gointernal/container/start.gointernal/container/start_test.gointernal/env/env.gointernal/ui/run.gointernal/ui/run_login.gointernal/ui/run_login_test.gointernal/ui/run_logout.go
Comment on lines
+25
to
+37
| Run: func(cmd *cobra.Command, args []string) { | ||
| rt, err := runtime.NewDockerRuntime() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| os.Exit(1) | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| func init() { | ||
| rootCmd.Version = version.Version() | ||
| rootCmd.SetVersionTemplate(versionLine() + "\n") | ||
| if err := runStart(cmd.Context(), rt, cfg); err != nil { | ||
| if !output.IsSilent(err) { | ||
| fmt.Fprintf(os.Stderr, "Error: %v\n", err) | ||
| } | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Check stderr write errors before exiting.
fmt.Fprintf results are ignored on Line 28 and Line 34. Please check and handle write errors explicitly.
🛠️ Minimal fix
if err != nil {
- fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+ if _, writeErr := fmt.Fprintf(os.Stderr, "Error: %v\n", err); writeErr != nil {
+ // best-effort logging; continue exiting with original error
+ }
os.Exit(1)
}
@@
if err := runStart(cmd.Context(), rt, cfg); err != nil {
if !output.IsSilent(err) {
- fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+ if _, writeErr := fmt.Fprintf(os.Stderr, "Error: %v\n", err); writeErr != nil {
+ // best-effort logging; continue exiting with original error
+ }
}
os.Exit(1)
}As per coding guidelines: "Errors returned by functions should always be checked unless in test files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/root.go` around lines 25 - 37, The fmt.Fprintf calls that write errors to
stderr after runtime.NewDockerRuntime() and after runStart() must have their
returned values checked; locate the two fmt.Fprintf(...) usages in the Run
function (surrounding runtime.NewDockerRuntime and the runStart error handling
that uses output.IsSilent) and replace the ignored write with a checked write
(capture the returned n, err := fmt.Fprintf(...)) and handle any write error
before calling os.Exit(1) — e.g., if fmt.Fprintf returns an error, attempt a
fallback write (such as fmt.Fprintln(os.Stderr, "failed to write to stderr:",
writeErr)) or log the write failure so the process still exits with the intended
status; ensure both occurrences are updated so no write errors are ignored.
Comment on lines
+63
to
+64
| cfg := env.Init() | ||
| return NewRootCmd(cfg).ExecuteContext(ctx) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmd/root.go | head -80Repository: localstack/lstk
Length of output: 2558
🏁 Script executed:
cat -n internal/env/env.go | head -100Repository: localstack/lstk
Length of output: 1185
🏁 Script executed:
rg -n 'func Init|config\.|viper\.|os\.Getenv|LookupEnv' internal/env/env.go -A 2 -B 1Repository: localstack/lstk
Length of output: 739
🏁 Script executed:
rg -n 'func initConfig|PreRunE' cmd/root.go -A 5Repository: localstack/lstk
Length of output: 360
🏁 Script executed:
fd -e go internal/config | head -20Repository: localstack/lstk
Length of output: 424
🏁 Script executed:
rg -n 'func Init' internal/config -A 10 | head -50Repository: localstack/lstk
Length of output: 666
🏁 Script executed:
rg -n 'func loadConfig|func firstExistingConfigPath' internal/config -A 15Repository: localstack/lstk
Length of output: 1763
🏁 Script executed:
cat -n internal/config/config.go | head -80Repository: localstack/lstk
Length of output: 2186
Move env.Init() to after config.Init() is guaranteed to complete, or restructure to wire env initialization in the command's PreRunE.
The current code captures env values at line 63 before config.Init() has loaded any config file (which happens later via PreRunE at line 24). This means cfg captures defaults set by env.Init() only, not values that may exist in the config file for api_endpoint, web_app_url, or keyring. When runStart (lines 67–72) uses these stale values, it won't reflect config-file overrides.
Either call config.Init() before env.Init(), or move env initialization into PreRunE to ensure config is loaded first—consistent with the guideline: "When adding a new command that depends on configuration, wire config initialization explicitly in that command (PreRunE: initConfig)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/root.go` around lines 63 - 64, The code calls env.Init() before
config.Init(), causing cfg to capture defaults instead of config-file overrides;
update NewRootCmd/command wiring so environment initialization runs after
configuration is loaded—either call config.Init() before env.Init() or move
env.Init() into the command's PreRunE (e.g., after initConfig runs) so runStart
and any use of cfg reflect config-file values; locate env.Init(), config.Init(),
NewRootCmd, PreRunE, and runStart in the diff and reorder or invoke env.Init()
from PreRunE (after initConfig) accordingly.
Comment on lines
+25
to
+29
| onProgress := func(msg string) { | ||
| fmt.Println(msg) | ||
| } | ||
|
|
||
| if err := container.Stop(cmd.Context(), rt, onProgress); err != nil { | ||
| fmt.Fprintln(os.Stderr, err) | ||
| os.Exit(1) | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| func init() { | ||
| rootCmd.AddCommand(stopCmd) | ||
| if err := container.Stop(cmd.Context(), rt, onProgress); err != nil { |
There was a problem hiding this comment.
Route stop progress/output via typed sinks, not callback-print wiring.
This segment wires onProgress func(string) and direct stdout printing in the command path; that couples presentation to command/domain flow instead of using internal/output event sinks selected at the command boundary.
As per coding guidelines "Keep CLI business logic out of cmd/ - CLI wiring only (Cobra framework) belongs in cmd/" and "Do not pass UI callbacks like onProgress func(...) through domain layers; prefer typed output events."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/stop.go` around lines 25 - 29, The stop command currently creates an
onProgress func and prints directly, then passes that callback into
container.Stop; instead, remove the UI callback from the domain call and route
progress via the typed internal/output event sinks at the cmd boundary: stop
building an event sink (or use the existing internal/output emitter) in the cmd/
stop handler, subscribe/send progress events there, and change the call site so
container.Stop(ctx, rt) no longer accepts onProgress; update container.Stop (and
any downstream domain functions) to emit typed progress events via the existing
output/event interfaces rather than receiving a UI callback (references:
onProgress, container.Stop, internal/output event sinks).
| fmt.Println(msg) | ||
| } | ||
| onProgress := func(msg string) { | ||
| fmt.Println(msg) |
There was a problem hiding this comment.
Check the write error from fmt.Println.
Line 26 drops the returned error from fmt.Println, which can hide broken-pipe/write failures in non-interactive usage.
As per coding guidelines "Errors returned by functions should always be checked unless in test files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/stop.go` at line 26, The call to fmt.Println(msg) drops its returned
error; change it to check and handle the error (e.g. if _, err :=
fmt.Println(msg); err != nil { return err } or log the error and os.Exit(1)
depending on the surrounding function's error-handling pattern) so write
failures (broken pipe, etc.) are not ignored; update the statement that
currently reads fmt.Println(msg) to perform the error check and propagate or
exit consistently with the caller.
Comment on lines
65
to
70
| _, err := a.tokenStorage.GetAuthToken() | ||
| if err != nil { | ||
| output.EmitSpinnerStop(a.sink) | ||
| if env.Vars.AuthToken != "" { | ||
| if a.authToken != "" { | ||
| output.EmitNote(a.sink, "Authenticated via LOCALSTACK_AUTH_TOKEN environment variable; unset it to log out") | ||
| return nil |
There was a problem hiding this comment.
Logout can report success while injected auth token still keeps auth active.
When keyring deletion succeeds and a.authToken is non-empty, the command reports full logout even though subsequent auth still resolves from the injected token.
💡 Suggested adjustment
if err := a.tokenStorage.DeleteAuthToken(); err != nil {
output.EmitSpinnerStop(a.sink)
return fmt.Errorf("failed to delete auth token: %w", err)
}
output.EmitSpinnerStop(a.sink)
output.EmitSuccess(a.sink, "Logged out successfully")
+ if a.authToken != "" {
+ output.EmitNote(a.sink, "Authenticated via LOCALSTACK_AUTH_TOKEN environment variable; unset it to fully log out")
+ }
return nilAlso applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/auth.go` around lines 65 - 70, The logout path currently treats
keyring deletion as a full logout even when an injected token (a.authToken)
remains set, causing a false success report; in the blocks around token
retrieval and keyring deletion (references: a.tokenStorage.GetAuthToken,
a.authToken, output.EmitNote), change the control flow so that if a.authToken !=
"" you do NOT report a successful logout — instead emit a clear warning/note
that an injected LOCALSTACK_AUTH_TOKEN remains active and return a non-success
result (or an explicit error/exit) so callers/users are informed that logout is
incomplete; apply the same fix to the other identical branch around lines 84-86.
silv-io
approved these changes
Mar 3, 2026
| AuthToken: os.Getenv("LOCALSTACK_AUTH_TOKEN"), | ||
| APIEndpoint: viper.GetString("api_endpoint"), | ||
| WebAppURL: viper.GetString("web_app_url"), | ||
| ForceFileKeyring: viper.GetString("keyring") == "file", |
| ) | ||
|
|
||
| func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink, platformClient api.PlatformAPI, interactive bool) error { | ||
| func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink, platformClient api.PlatformAPI, authToken string, forceFileBackend bool, webAppURL string, interactive bool) error { |
Member
There was a problem hiding this comment.
q: here we're using forceFileBackend and in the env forceFileKeyring. Is this intentional?
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.
Switched to a more idiomatic go and stateless dependency injection:
closes PRO-226