Skip to content

Conversation

@jyecusch
Copy link
Member

Adds a debug flag to enable verbose logging of API requests and responses.

This feature helps troubleshoot issues by providing detailed information about the communication between the CLI and the backend server.

The debug flag is added as a persistent flag to the root command, allowing it to be used with any subcommand.

The API client and the WorkOS client are updated to log requests and responses when the debug flag is enabled.

@jyecusch jyecusch marked this pull request as draft September 12, 2025 04:28
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
cli/internal/workos/http/client.go (3)

230-249: Close response body after reads

GetJWKs reads body but never closes it. Add a defer to prevent leaks.

 response, err := h.get(jwkPath)
 if err != nil {
   return nil, err
 }
+defer response.Body.Close()

252-281: Close response body in AuthenticateWithCode

Prevent resource leak.

 response, err := h.post("/user_management/authenticate", body)
 if err != nil {
   return nil, err
 }
+defer response.Body.Close()

351-381: Close response body in AuthenticateWithRefreshToken

Same leak pattern here.

 response, err := h.post("/auth/refresh", body)
 if err != nil {
   return nil, err
 }
+defer response.Body.Close()
🧹 Nitpick comments (5)
cli/internal/config/config.go (1)

19-19: Config.Debug added — OK, but consider not persisting it by default

If Debug is meant to be ephemeral (CLI session only), saving the config later may persist it unintentionally.

Apply this small guard in Save to omit Debug from writes:

 func (c *Config) Save(global bool) error {
   var configMap map[string]interface{}
   err := mapstructure.Decode(c, &configMap)
   if err != nil {
     return fmt.Errorf("failed to decode config struct: %w", err)
   }
+  // Don't persist ephemeral flags
+  delete(configMap, "debug")
cli/cmd/root.go (1)

25-27: Enable debug from flag — good wiring

Works for all subcommands. Optionally print a one-liner to stderr so users notice debug mode is on.

 if debugFlag, _ := cmd.Flags().GetBool("debug"); debugFlag {
   conf.SetDebug(true)
+  fmt.Fprintln(os.Stderr, "[DEBUG] Verbose logging enabled (sensitive headers redacted).")
 }

Note: add os to imports.

cli/internal/workos/workos.go (1)

39-41: Avoid shadowing the imported package name

Local variable config shadows the config package; rename to cfg for clarity.

- config := do.MustInvoke[*config.Config](inj)
- sugaServerUrl := config.GetSugaServerUrl()
+ cfg := do.MustInvoke[*config.Config](inj)
+ sugaServerUrl := cfg.GetSugaServerUrl()
 ...
- httpClient := http.NewHttpClient("", config, opts...)
+ httpClient := http.NewHttpClient("", cfg, opts...)
cli/internal/api/api.go (1)

93-96: Consider using a client with timeouts instead of http.DefaultClient

Default client has no timeout; a hung server can stall the CLI.

Define an http.Client with a sane timeout (e.g., 30s) and reuse it in SugaApiClient.

Also applies to: 145-149

cli/internal/workos/http/client.go (1)

289-306: Optional: share a single helper to join base URL + path

Small consistency nit: you alternate between JoinPath(path) and JoinPath("/path"). Not wrong, but a helper reduces chances of double slashes.

Also applies to: 320-337, 513-534

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cb718b and 6953f77.

📒 Files selected for processing (5)
  • cli/cmd/root.go (1 hunks)
  • cli/internal/api/api.go (5 hunks)
  • cli/internal/config/config.go (2 hunks)
  • cli/internal/workos/http/client.go (5 hunks)
  • cli/internal/workos/workos.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/internal/workos/workos.go (1)
cli/internal/workos/http/client.go (1)
  • NewHttpClient (109-134)
cli/internal/workos/http/client.go (1)
cli/internal/config/config.go (1)
  • Config (16-20)
🔇 Additional comments (4)
cli/internal/config/config.go (1)

97-99: Setter looks good

Simple and clear. No issues.

cli/cmd/root.go (1)

34-34: Flag UX — clear

Help text is explicit and matches behavior.

cli/internal/workos/workos.go (1)

55-55: Updated NewHttpClient call — OK

Signature change handled correctly and passes config through.

cli/internal/workos/http/client.go (1)

306-317: Debug calls are fine once redaction is in place

With the redaction changes above, these call sites are safe.

Also applies to: 334-345, 530-541

Adds a debug flag to enable verbose logging of API requests and responses.

This feature helps troubleshoot issues by providing detailed information about the communication between the CLI and the backend server.

The debug flag is added as a persistent flag to the root command, allowing it to be used with any subcommand.

The API client and the WorkOS client are updated to log requests and responses when the debug flag is enabled.
@nitrictech nitrictech deleted a comment from coderabbitai bot Sep 12, 2025
@jyecusch jyecusch changed the title fix(cli): add debug logging flag feat(cli): add debug logging flag Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants