fix: address review findings across packages

- Handle defaults.Set error in config.Init
- Cache terminal theme to avoid repeated capability queries
- Return error instead of panicking in OpenBrowser
- Restore original Out after StopPager, fix Bold writing to stdout
- Sanitize repo path to prevent traversal in version cache
- Only set CORS headers when origin is allowed
- Use atomic counter for unique error reference IDs
- Sort command groups for deterministic help output
- Run onStop for successful onStart hooks on partial failure
- Use dynamic ports in server and app tests to prevent CI flakes
- Align Go version in docs with go.mod (1.25)
- Remove unused tarURL field, fix PreRun preservation in AddJSONFlags
- Handle embedded structs in StructExportData
- Rename isTTYWriter to isTTY for clarity

Author: ravisuhag

.github/workflows/lint.yaml

@@ -26,4 +26,4 @@ jobs:
       - name: Run linter
         uses: golangci/golangci-lint-action@v7
         with:
-          version: v2.11
+          version: v2.11

MIGRATION.md

@@ -4,10 +4,10 @@ This guide covers migrating from the previous salt version to the new structure.
 
 ## Go version
 
-Update `go.mod` to require Go 1.24:
+Update `go.mod` to require Go 1.25:
 
 ```text
-go 1.24
+go 1.25
 ```
 
 ## Packages removed

README.md

@@ -69,7 +69,7 @@ func main() {
 go get github.com/raystack/salt
 ```
 
-Requires Go 1.24+.
+Requires Go 1.25+.
 
 ## Packages
 

app/app.go

@@ -64,9 +64,11 @@ func (a *App) Start(ctx context.Context) error {
 		a.telClean = cleanup
 	}
 
-	// Run onStart hooks.
-	for _, fn := range a.onStart {
+	// Run onStart hooks. If one fails, run onStop for previously
+	// successful hooks before returning.
+	for i, fn := range a.onStart {
 		if err := fn(ctx); err != nil {
+			a.stopHooks(context.Background(), i)
 			a.cleanup()
 			return fmt.Errorf("app on_start: %w", err)
 		}
@@ -91,12 +93,18 @@ func (a *App) Logger() *slog.Logger {
 }
 
 func (a *App) stop(ctx context.Context) {
-	for _, fn := range a.onStop {
-		if err := fn(ctx); err != nil {
+	a.stopHooks(ctx, len(a.onStop))
+	a.cleanup()
+}
+
+// stopHooks runs onStop hooks for the first n hooks (used for partial cleanup
+// when an onStart hook fails partway through).
+func (a *App) stopHooks(ctx context.Context, n int) {
+	for i := 0; i < n && i < len(a.onStop); i++ {
+		if err := a.onStop[i](ctx); err != nil {
 			a.logger.Error("app on_stop hook error", "error", err)
 		}
 	}
-	a.cleanup()
 }
 
 func (a *App) cleanup() {

app/app_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"net"
 	"net/http"
 	"testing"
 	"time"
@@ -16,6 +17,18 @@ import (
 
 func nopLogger() *slog.Logger { return slog.New(slog.DiscardHandler) }
 
+// freeAddr returns a "127.0.0.1:<port>" string using a port that is free at
+// the time of the call. There is a small TOCTOU window, but it eliminates
+// hardcoded-port flakes in CI.
+func freeAddr(t *testing.T) string {
+	t.Helper()
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	addr := ln.Addr().String()
+	ln.Close()
+	return addr
+}
+
 func TestNew(t *testing.T) {
 	t.Run("creates app with defaults", func(t *testing.T) {
 		a, err := app.New()
@@ -44,10 +57,11 @@ func TestNew(t *testing.T) {
 func TestAppStartAndShutdown(t *testing.T) {
 	t.Run("starts with health check and h2c by default", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
+		addr := freeAddr(t)
 
 		a, err := app.New(
 			app.WithLogger(nopLogger()),
-			app.WithAddr("127.0.0.1:18950"),
+			app.WithAddr(addr),
 		)
 		require.NoError(t, err)
 
@@ -57,7 +71,7 @@ func TestAppStartAndShutdown(t *testing.T) {
 		time.Sleep(100 * time.Millisecond)
 
 		// Health check should be on by default at /ping
-		resp, err := http.Get("http://127.0.0.1:18950/ping")
+		resp, err := http.Get("http://" + addr + "/ping")
 		require.NoError(t, err)
 		defer resp.Body.Close()
 		assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -74,10 +88,11 @@ func TestAppStartAndShutdown(t *testing.T) {
 
 	t.Run("runs onStart hooks", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
+		addr := freeAddr(t)
 
 		var hookRan bool
 		a, err := app.New(
-			app.WithAddr("127.0.0.1:18951"),
+			app.WithAddr(addr),
 			app.WithOnStart(func(_ context.Context) error {
 				hookRan = true
 				return nil
@@ -96,10 +111,11 @@ func TestAppStartAndShutdown(t *testing.T) {
 
 	t.Run("runs onStop hooks", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
+		addr := freeAddr(t)
 
 		var hookRan bool
 		a, err := app.New(
-			app.WithAddr("127.0.0.1:18952"),
+			app.WithAddr(addr),
 			app.WithOnStop(func(_ context.Context) error {
 				hookRan = true
 				return nil
@@ -119,10 +135,11 @@ func TestAppStartAndShutdown(t *testing.T) {
 	t.Run("serves custom handler", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
+		addr := freeAddr(t)
 
 		a, err := app.New(
 			app.WithLogger(nopLogger()),
-			app.WithAddr("127.0.0.1:18953"),
+			app.WithAddr(addr),
 			app.WithHandler("/hello", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 				fmt.Fprint(w, "world")
 			})),
@@ -132,7 +149,7 @@ func TestAppStartAndShutdown(t *testing.T) {
 		go a.Start(ctx)
 		time.Sleep(100 * time.Millisecond)
 
-		resp, err := http.Get("http://127.0.0.1:18953/hello")
+		resp, err := http.Get("http://" + addr + "/hello")
 		require.NoError(t, err)
 		defer resp.Body.Close()
 
@@ -145,6 +162,7 @@ func TestAppStartAndShutdown(t *testing.T) {
 	t.Run("applies explicit middleware", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
+		addr := freeAddr(t)
 
 		addHeader := func(next http.Handler) http.Handler {
 			return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -155,7 +173,7 @@ func TestAppStartAndShutdown(t *testing.T) {
 
 		a, err := app.New(
 			app.WithLogger(nopLogger()),
-			app.WithAddr("127.0.0.1:18954"),
+			app.WithAddr(addr),
 			app.WithHTTPMiddleware(addHeader),
 			app.WithHandler("/test", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 				w.WriteHeader(http.StatusOK)
@@ -166,7 +184,7 @@ func TestAppStartAndShutdown(t *testing.T) {
 		go a.Start(ctx)
 		time.Sleep(100 * time.Millisecond)
 
-		resp, err := http.Get("http://127.0.0.1:18954/test")
+		resp, err := http.Get("http://" + addr + "/test")
 		require.NoError(t, err)
 		defer resp.Body.Close()
 
@@ -176,9 +194,10 @@ func TestAppStartAndShutdown(t *testing.T) {
 	})
 
 	t.Run("onStart failure returns error and runs cleanup", func(t *testing.T) {
+		addr := freeAddr(t)
 		var cleanupRan bool
 		a, err := app.New(
-			app.WithAddr("127.0.0.1:18955"),
+			app.WithAddr(addr),
 			app.WithOnStart(func(_ context.Context) error {
 				return fmt.Errorf("migration failed")
 			}),

cli/commander/layout.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/muesli/termenv"
@@ -104,7 +105,14 @@ func buildHelpEntries(cmd *cobra.Command) []helpEntry {
 		// No groups defined — show all commands under "Core commands"
 		helpEntries = append(helpEntries, helpEntry{"Core commands", strings.Join(ungroupedCommands, "\n")})
 	} else {
-		for group, cmds := range groupCommands {
+		// Sort group names for deterministic output.
+		groupNames := make([]string, 0, len(groupCommands))
+		for group := range groupCommands {
+			groupNames = append(groupNames, group)
+		}
+		sort.Strings(groupNames)
+		for _, group := range groupNames {
+			cmds := groupCommands[group]
 			helpEntries = append(helpEntries, helpEntry{fmt.Sprintf("%s commands", toTitle(group)), strings.Join(cmds, "\n")})
 		}
 		if len(ungroupedCommands) > 0 {

cli/export.go

@@ -72,12 +72,17 @@ func AddJSONFlags(cmd *cobra.Command, exportTarget *Exporter, fields []string) {
 	})
 
 	// Validate field names and set the exporter.
-	oldPreRun := cmd.PreRunE
+	// Preserve both PreRunE and PreRun (non-error variant).
+	oldPreRunE := cmd.PreRunE
+	oldPreRun := cmd.PreRun
+	cmd.PreRun = nil // avoid cobra running both
 	cmd.PreRunE = func(c *cobra.Command, args []string) error {
-		if oldPreRun != nil {
-			if err := oldPreRun(c, args); err != nil {
+		if oldPreRunE != nil {
+			if err := oldPreRunE(c, args); err != nil {
 				return err
 			}
+		} else if oldPreRun != nil {
+			oldPreRun(c, args)
 		}
 
 		jsonFlag := c.Flags().Lookup("json")
@@ -221,16 +226,24 @@ func (e *jsonExporter) extractData(v reflect.Value) any {
 }
 
 // fieldByTag finds a struct field whose `json` tag matches the given name.
+// It searches embedded structs recursively.
 func fieldByTag(v reflect.Value, name string) reflect.Value {
 	t := v.Type()
 	for i := 0; i < t.NumField(); i++ {
-		tag := t.Field(i).Tag.Get("json")
+		sf := t.Field(i)
+		tag := sf.Tag.Get("json")
 		if idx := strings.IndexByte(tag, ','); idx >= 0 {
 			tag = tag[:idx]
 		}
 		if strings.EqualFold(tag, name) {
 			return v.Field(i)
 		}
+		// Recurse into embedded (anonymous) struct fields.
+		if sf.Anonymous && sf.Type.Kind() == reflect.Struct {
+			if result := fieldByTag(v.Field(i), name); result.IsValid() {
+				return result
+			}
+		}
 	}
 	return reflect.Value{}
 }

cli/iostreams.go

@@ -37,6 +37,7 @@ type IOStreams struct {
 
 	pager        *terminal.Pager
 	pagerStarted bool
+	origOut      io.Writer
 
 	// lazily created
 	output   *printer.Output
@@ -45,14 +46,14 @@ type IOStreams struct {
 
 // System creates IOStreams wired to the real terminal.
 func System() *IOStreams {
-	outTTY := isTTYWriter(os.Stdout)
+	outTTY := isTTY(os.Stdout)
 	return &IOStreams{
 		In:           os.Stdin,
 		Out:          os.Stdout,
 		ErrOut:       os.Stderr,
-		inTTY:        isTTYWriter(os.Stdin),
+		inTTY:        isTTY(os.Stdin),
 		outTTY:       outTTY,
-		errTTY:       isTTYWriter(os.Stderr),
+		errTTY:       isTTY(os.Stderr),
 		colorEnabled: outTTY && !termenv.EnvNoColor(),
 	}
 }
@@ -127,6 +128,7 @@ func (s *IOStreams) StartPager() error {
 	if err := p.Start(); err != nil {
 		return err
 	}
+	s.origOut = s.Out
 	s.Out = p.Out
 	s.pager = p
 	s.pagerStarted = true
@@ -139,6 +141,11 @@ func (s *IOStreams) StopPager() {
 	if s.pager != nil && s.pagerStarted {
 		s.pager.Stop()
 		s.pagerStarted = false
+		if s.origOut != nil {
+			s.Out = s.origOut
+			s.origOut = nil
+			s.output = nil // invalidate cached Output
+		}
 	}
 }
 
@@ -158,6 +165,6 @@ func (s *IOStreams) Prompter() prompt.Prompter {
 	return s.prompter
 }
 
-func isTTYWriter(f *os.File) bool {
+func isTTY(f *os.File) bool {
 	return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd())
 }

cli/printer/printer.go

@@ -26,6 +26,10 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
+// cachedTheme is computed once at init to avoid querying terminal capabilities
+// on every color function call.
+var cachedTheme = newTheme()
+
 // Output handles all terminal output for a CLI command.
 //
 // Data output (Table, JSON, YAML, Println) goes to the primary writer (stdout).
@@ -77,9 +81,9 @@ func (o *Output) Info(msg string) {
 	fmt.Fprintln(o.errW, o.color(o.theme.Cyan, msg))
 }
 
-// Bold prints a bold message.
+// Bold prints a bold message to stderr.
 func (o *Output) Bold(msg string) {
-	fmt.Fprintln(o.w, termenv.String(msg).Bold().String())
+	fmt.Fprintln(o.errW, termenv.String(msg).Bold().String())
 }
 
 // Print prints a plain message.
@@ -224,25 +228,25 @@ func (o *Output) Progress(max int, description string) *progressbar.ProgressBar
 // --- Formatting helpers (return styled strings for composition) ---
 
 // Green returns text styled in green.
-func Green(t string) string { return colorize(newTheme().Green, t) }
+func Green(t string) string { return colorize(cachedTheme.Green, t) }
 
 // Yellow returns text styled in yellow.
-func Yellow(t string) string { return colorize(newTheme().Yellow, t) }
+func Yellow(t string) string { return colorize(cachedTheme.Yellow, t) }
 
 // Cyan returns text styled in cyan.
-func Cyan(t string) string { return colorize(newTheme().Cyan, t) }
+func Cyan(t string) string { return colorize(cachedTheme.Cyan, t) }
 
 // Red returns text styled in red.
-func Red(t string) string { return colorize(newTheme().Red, t) }
+func Red(t string) string { return colorize(cachedTheme.Red, t) }
 
 // Grey returns text styled in grey.
-func Grey(t string) string { return colorize(newTheme().Grey, t) }
+func Grey(t string) string { return colorize(cachedTheme.Grey, t) }
 
 // Blue returns text styled in blue.
-func Blue(t string) string { return colorize(newTheme().Blue, t) }
+func Blue(t string) string { return colorize(cachedTheme.Blue, t) }
 
 // Magenta returns text styled in magenta.
-func Magenta(t string) string { return colorize(newTheme().Magenta, t) }
+func Magenta(t string) string { return colorize(cachedTheme.Magenta, t) }
 
 // --- Formatted color helpers ---