Merged
Conversation
carole-lavillonniere
changed the title
📝 WalkthroughWalkthroughRefactors CLI from init-time globals to constructor functions and returns an explicit *env.Env from env.Init(); commands and runtime entrypoints now accept cfg and explicit parameters (API endpoint, auth token, keyring flag, web app URL). Auth, client, UI, and container call sites updated to use injected dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as NewRootCmd
participant Env as env.Init()
participant Client as NewPlatformClient(apiEndpoint)
participant Auth as auth.New / TokenStorage
participant UI as ui.RunLogin / ui.Run
participant Container as container.Start
User->>CLI: invoke command (start / login / logout)
CLI->>Env: env.Init()
Env-->>CLI: *env.Env (cfg)
CLI->>Client: NewPlatformClient(cfg.APIEndpoint)
Client-->>CLI: PlatformClient
CLI->>UI: ui.RunLogin/... (platformClient, cfg.AuthToken, cfg.ForceFileKeyring, cfg.WebAppURL)
UI->>Auth: New(TokenStorage(cfg.ForceFileKeyring), platformClient, cfg.AuthToken, cfg.WebAppURL)
Auth->>Auth: GetToken / interactive flow
Auth-->>UI: token or error
UI->>Container: container.Start(..., platformClient, cfg.AuthToken, cfg.ForceFileKeyring, cfg.WebAppURL, interactive)
Container-->>UI: started / error
UI-->>User: render UI output / exit
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", |
internal/container/start.go
Outdated
| ) | ||
|
|
||
| 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?
Collaborator
Author
There was a problem hiding this comment.
Good catch 🙃 Renamed all to forceFileKeyring
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/run_login_test.go (1)
180-229:⚠️ Potential issue | 🔴 CriticalTestLoginFlow_DeviceFlowCancelWithCtrlC uses stale API signatures and will fail to compile.
This test was not updated to match the new signatures:
- Line 186:
env.Init()is called but theenvpackage is not imported- Line 197:
api.NewPlatformClient()uses the old parameterless signature (should beNewPlatformClient(mockServer.URL))- Line 201:
auth.New(...)uses the old 4-parameter signature instead of the new 6-parameter signature🐛 Proposed fix
func TestLoginFlow_DeviceFlowCancelWithCtrlC(t *testing.T) { mockServer := createMockAPIServer(t, "", false) defer mockServer.Close() - t.Setenv("LSTK_API_ENDPOINT", mockServer.URL) - t.Setenv("LOCALSTACK_AUTH_TOKEN", "") - env.Init() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() ctrl := gomock.NewController(t) mockStorage := auth.NewMockAuthTokenStorage(ctrl) mockStorage.EXPECT().GetAuthToken().Return("", errors.New("no token")) tm := teatest.NewTestModel(t, NewApp("test", "", "", cancel), teatest.WithInitialTermSize(120, 40)) sender := testModelSender{tm: tm} - platformClient := api.NewPlatformClient() + platformClient := api.NewPlatformClient(mockServer.URL) errCh := make(chan error, 1) go func() { - a := auth.New(output.NewTUISink(sender), platformClient, mockStorage, true) + a := auth.New(output.NewTUISink(sender), platformClient, mockStorage, "", mockServer.URL, true) _, err := a.GetToken(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_login_test.go` around lines 180 - 229, The test TestLoginFlow_DeviceFlowCancelWithCtrlC uses outdated signatures: add the missing import for the env package and keep the call to env.Init(), change the platform client construction to pass the mock server URL by replacing api.NewPlatformClient() with api.NewPlatformClient(mockServer.URL), and update the auth.New(...) call to match the current 6-parameter signature (open the auth.New declaration and supply the two additional arguments required by the new API—e.g., add the missing context/opts or logger parameters from the test environment—so the call uses output.NewTUISink(sender), platformClient, mockStorage, true plus the two new args expected by auth.New).
🧹 Nitpick comments (2)
internal/auth/auth.go (1)
32-40: Silently ignoring storage errors when falling back to authToken.Line 34 checks
err == nil && token != "", which correctly falls through when there's no token. However, iftokenStorage.GetAuthToken()returns an error other thanErrTokenNotFound(e.g., keyring access failure), the code silently falls back toauthTokeninstead of surfacing the error. This could mask underlying issues.Consider explicitly handling only the "not found" case:
💡 Suggested improvement
func (a *Auth) GetToken(ctx context.Context) (string, error) { - if token, err := a.tokenStorage.GetAuthToken(); err == nil && token != "" { + token, err := a.tokenStorage.GetAuthToken() + if err == nil && token != "" { return token, nil } + if err != nil && !errors.Is(err, ErrTokenNotFound) { + output.EmitWarning(a.sink, fmt.Sprintf("could not read token from keyring: %v", err)) + } if token := a.authToken; token != "" { return token, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/auth.go` around lines 32 - 40, In Auth.GetToken, don’t silently ignore errors from tokenStorage.GetAuthToken; after calling tokenStorage.GetAuthToken() check the returned error and only treat the "not found" case as non-fatal—if err != nil and err is not ErrTokenNotFound, return the error (or wrap and return) instead of falling back to a.authToken; keep the existing behavior for err == nil && token != "" and for the separate a.authToken fallback.internal/ui/run_logout.go (1)
14-30: Consider documenting whywebAppURLis empty for logout.The
auth.Newcall at line 30 passes an empty string forwebAppURL. This works becauseallowLoginisfalse, so the login flow (which useswebAppURL) will never be triggered. However, this implicit dependency is subtle.A brief comment would clarify the intent:
// webAppURL is empty because logout never triggers interactive login (allowLogin=false) a := auth.New(output.NewTUISink(programSender{p: p}), platformClient, tokenStorage, authToken, "", false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_logout.go` around lines 14 - 30, The call to auth.New in RunLogout passes an empty string for webAppURL which relies on the fact that allowLogin is false; add a short clarifying comment (or assign a named variable like webAppURL := "" with a comment) immediately above the auth.New(...) call to state that webAppURL is intentionally empty because logout never triggers interactive login (allowLogin=false), referencing RunLogout and the auth.New call so future readers understand the implicit dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/ui/run_login_test.go`:
- Around line 180-229: The test TestLoginFlow_DeviceFlowCancelWithCtrlC uses
outdated signatures: add the missing import for the env package and keep the
call to env.Init(), change the platform client construction to pass the mock
server URL by replacing api.NewPlatformClient() with
api.NewPlatformClient(mockServer.URL), and update the auth.New(...) call to
match the current 6-parameter signature (open the auth.New declaration and
supply the two additional arguments required by the new API—e.g., add the
missing context/opts or logger parameters from the test environment—so the call
uses output.NewTUISink(sender), platformClient, mockStorage, true plus the two
new args expected by auth.New).
---
Nitpick comments:
In `@internal/auth/auth.go`:
- Around line 32-40: In Auth.GetToken, don’t silently ignore errors from
tokenStorage.GetAuthToken; after calling tokenStorage.GetAuthToken() check the
returned error and only treat the "not found" case as non-fatal—if err != nil
and err is not ErrTokenNotFound, return the error (or wrap and return) instead
of falling back to a.authToken; keep the existing behavior for err == nil &&
token != "" and for the separate a.authToken fallback.
In `@internal/ui/run_logout.go`:
- Around line 14-30: The call to auth.New in RunLogout passes an empty string
for webAppURL which relies on the fact that allowLogin is false; add a short
clarifying comment (or assign a named variable like webAppURL := "" with a
comment) immediately above the auth.New(...) call to state that webAppURL is
intentionally empty because logout never triggers interactive login
(allowLogin=false), referencing RunLogout and the auth.New call so future
readers understand the implicit dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 88e95534-28be-42b4-bec0-3b53df4c90c2
📒 Files selected for processing (6)
internal/auth/auth.gointernal/auth/login.gointernal/ui/run.gointernal/ui/run_login.gointernal/ui/run_login_test.gointernal/ui/run_logout.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/auth/login.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/auth/token_storage.go (1)
49-60: Redundant retry whenforceFileKeyringis true.When
forceFileKeyringis true,AllowedBackendsis already set toFileBackendat line 50. Ifkeyring.Openfails, the fallback at lines 55-56 sets the same configuration and retries, which will fail identically.♻️ Suggested improvement
if forceFileKeyring { keyringConfig.AllowedBackends = []keyring.BackendType{keyring.FileBackend} } ring, err := keyring.Open(keyringConfig) - if err != nil { + if err != nil && !forceFileKeyring { keyringConfig.AllowedBackends = []keyring.BackendType{keyring.FileBackend} ring, err = keyring.Open(keyringConfig) - if err != nil { - return nil, fmt.Errorf("failed to open keyring: %w", err) - } + } + if err != nil { + return nil, fmt.Errorf("failed to open keyring: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/token_storage.go` around lines 49 - 60, The current code redundantly retries with the same configuration when forceFileKeyring is true; update the logic around keyring.Open so that if forceFileKeyring is true you set keyringConfig.AllowedBackends = []keyring.BackendType{keyring.FileBackend} and call keyring.Open once and return the error if it fails, and only perform the fallback reset-and-retry (setting AllowedBackends to FileBackend and reopening) when forceFileKeyring is false; adjust the block that uses forceFileKeyring, keyringConfig, keyring.Open, and ring to avoid resetting AllowedBackends and retrying in the forced-file case.internal/ui/run_logout.go (1)
14-16: Unused context variable.The context
ctxcreated at line 15 is assigned to_and never used. If the context isn't needed for cancellation propagation withinRunLogout, consider removing its creation or using it appropriately.-func RunLogout(parentCtx context.Context, platformClient api.PlatformAPI, authToken string, forceFileKeyring bool) error { - _, cancel := context.WithCancel(parentCtx) +func RunLogout(_ context.Context, platformClient api.PlatformAPI, authToken string, forceFileKeyring bool) error { + _, cancel := context.WithCancel(context.Background())Alternatively, if the parent context should be respected for cancellation:
- _, cancel := context.WithCancel(parentCtx) + ctx, cancel := context.WithCancel(parentCtx)And then pass
ctxto any operations that should respect cancellation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_logout.go` around lines 14 - 16, In RunLogout, the call to context.WithCancel(parentCtx) discards the returned context (assigned to _) and only uses cancel, creating an unused ctx; either remove the context.WithCancel call and the defer cancel() entirely if no cancellation is needed, or capture the returned context into a variable (e.g., ctx) and pass that ctx to any operations inside RunLogout that should honor cancellation (replace uses of parentCtx where appropriate) and keep defer cancel() to ensure proper cleanup; specifically update the context.WithCancel usage in RunLogout to either be eliminated or to assign to ctx and use ctx in subsequent API calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/auth/token_storage.go`:
- Around line 49-60: The current code redundantly retries with the same
configuration when forceFileKeyring is true; update the logic around
keyring.Open so that if forceFileKeyring is true you set
keyringConfig.AllowedBackends = []keyring.BackendType{keyring.FileBackend} and
call keyring.Open once and return the error if it fails, and only perform the
fallback reset-and-retry (setting AllowedBackends to FileBackend and reopening)
when forceFileKeyring is false; adjust the block that uses forceFileKeyring,
keyringConfig, keyring.Open, and ring to avoid resetting AllowedBackends and
retrying in the forced-file case.
In `@internal/ui/run_logout.go`:
- Around line 14-16: In RunLogout, the call to context.WithCancel(parentCtx)
discards the returned context (assigned to _) and only uses cancel, creating an
unused ctx; either remove the context.WithCancel call and the defer cancel()
entirely if no cancellation is needed, or capture the returned context into a
variable (e.g., ctx) and pass that ctx to any operations inside RunLogout that
should honor cancellation (replace uses of parentCtx where appropriate) and keep
defer cancel() to ensure proper cleanup; specifically update the
context.WithCancel usage in RunLogout to either be eliminated or to assign to
ctx and use ctx in subsequent API calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ea677c3e-3d00-4893-9f1e-8fa01086f61d
📒 Files selected for processing (7)
CLAUDE.mdinternal/auth/token_storage.gointernal/container/start.gointernal/ui/run.gointernal/ui/run_login.gointernal/ui/run_login_test.gointernal/ui/run_logout.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ui/run.go
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