refactor: evolve salt into raystack service framework#85
Open
refactor: evolve salt into raystack service framework#85
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a new app lifecycle manager ( Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant App
participant Telemetry
participant OnStartHooks
participant Server
participant OnStopHooks
User->>CLI: invoke (app.Run / CLI start)
CLI->>App: New(opts...) / Run(opts...)
App->>Telemetry: store config / defer init until Start
App->>App: Start(ctx)
App->>Telemetry: initOTLP / Init telemetry
Telemetry-->>App: telemetry cleanup func
App->>OnStartHooks: execute hooks (in order)
OnStartHooks-->>App: hook results
alt hook error
App->>App: run partial onStop hooks
App->>Telemetry: cleanup()
App-->>CLI: return error
else no error
App->>Server: build server with server.Options
Server->>Server: Start serving (goroutine)
App-->>CLI: running (blocks until ctx cancelled)
User->>App: send SIGINT/SIGTERM (context cancelled)
App->>Server: Shutdown(ctx with gracePeriod)
Server-->>App: shutdown result
App->>OnStopHooks: execute hooks
OnStopHooks-->>App: stop results
App->>Telemetry: cleanup()
App-->>CLI: exit
end
|
ravisuhag
force-pushed
the
refactor/salt-evolution
branch
4 times, most recently
from
a218a9e to
d4d0697
Compare
This was referenced Apr 18, 2026
Package moves: - observability/ → telemetry/ - cli/terminator → cli/terminal - cli/prompter → cli/prompt - cli/releaser → cli/version Packages dropped (zero consumers across raystack projects): - observability/logger — use *slog.Logger from stdlib - observability/otelgrpc — use otelconnect - observability/otelhttpclient — use otelhttp - server/mux — replaced by new server package - db/ — projects use their own DB library - auth/oidc — incomplete, tracked in #86 - auth/audit — too simple for real use, tracked in #87 - testing/dockertestx — projects use ory/dockertest directly - cli/printer old files — rewritten in later commit Fixes in existing code: - cobra.ExactValidArgs → cobra.MatchAll (deprecated) - strings.Replace → strings.ReplaceAll - revive indent-error-flow in spa/handler.go - pkg/errors → fmt.Errorf, cli/safeexec → exec.LookPath
New server package replacing server/mux. Single port, h2c by default:
srv := server.New(
server.WithAddr(":8080"),
server.WithHandler("/api/", handler),
)
srv.Start(ctx)
Defaults: h2c enabled, health check at /ping, 10s grace period.
Supports HTTP middleware, read/write/idle timeouts, graceful shutdown.
ConnectRPC interceptors + net/http middleware: - recovery, requestid, requestlog, errorz, cors Chain builders: Default(logger) for Connect, DefaultHTTP(logger) for HTTP.
app.Run(
app.WithLogger(slog.Default()),
app.WithHTTPMiddleware(middleware.DefaultHTTP(slog.Default())),
app.WithHandler("/api/", handler),
app.WithAddr(cfg.Addr),
)
No hidden magic — middleware is explicit. WithServer() passthrough
for any server option. WithOnStart/WithOnStop hooks for lifecycle.
cli.Execute(
cli.Name("frontier"),
cli.Version("0.1.0", "raystack/frontier"),
cli.Commands(serverCmd, configCmd),
)
Printer rewritten as Output type. CLI deps upgraded: survey/v2 → huh,
tablewriter → stdlib, glamour v0.3 → v1.0.
- validator v9 → v10, go-defaults → creasty/defaults - Remove fmt.Println on missing config file
CI: v1.60 → v2.11 (action v6 → v7). Drop errcheck linter.
README rewritten. MIGRATION.md covers all breaking changes. GoDoc examples in all packages.
ravisuhag
force-pushed
the
refactor/salt-evolution
branch
from
a72bf38 to
e2df2b3
Compare
cli.Init(rootCmd, opts...) enhances an existing cobra command instead
of creating one. The developer owns the root command — salt adds
help, completion, reference docs, version check, and output/prompter
context.
rootCmd := &cobra.Command{Use: "frontier", Short: "identity management"}
rootCmd.PersistentFlags().StringP("host", "h", "", "API host")
rootCmd.AddCommand(serverCmd, userCmd)
cli.Init(rootCmd,
cli.Version("0.1.0", "raystack/frontier"),
)
rootCmd.Execute()
This replaces cli.Execute() which couldn't support persistent flags,
PersistentPreRunE, or custom annotations on the root command.
Also fixes stale app doc comment mentioning "database".
- Init adds completion, reference, version commands - Version command not added without option - Output/Prompter return fallback when context not set - Fix nil context panic in Output/Prompter extractors
Verifies that when an OnStart hook fails, Start() returns the error and OnStop hooks do not run (only internal cleanup like telemetry flush executes).
Replaces ~50 lines of identical boilerplate in every raystack CLI:
rootCmd.AddCommand(cli.ConfigCommand("frontier", &Config{}))
Adds "config init" (creates config file with defaults) and
"config list" (shows current config as JSON). Uses config.WithAppConfig
for standard file location (~/.config/raystack/<app>.yml).
ravisuhag
force-pushed
the
refactor/salt-evolution
branch
from
c979b41 to
8a66841
Compare
- OpenTelemetry v1.31 → v1.43
- gRPC v1.67 → v1.80
- Viper v1.19 → v1.21
- testify v1.9 → v1.11
- go-version v1.3 → v1.9
- pflag v1.0.9 → v1.0.10
- jsondiff v0.7.0 → v0.7.1
- golang.org/x/{text,net,crypto,sys,term} to latest
- protobuf, genproto to latest
ravisuhag
force-pushed
the
refactor/salt-evolution
branch
from
ae9d123 to
d66d0fc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/version/release.go (1)
120-120:⚠️ Potential issue | 🟡 MinorCapitalize the user-facing update sentence.
The returned message starts a second sentence with lowercase
consider.Proposed copy fix
- return fmt.Sprintf("A new release (%s) is available. consider updating to latest version.", info.Version) + return fmt.Sprintf("A new release (%s) is available. Consider updating to latest version.", info.Version)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/version/release.go` at line 120, The user-facing message returned by the fmt.Sprintf call in release.go uses a lowercase "consider" starting the second sentence; update the string passed to fmt.Sprintf to capitalize "Consider" so it reads "A new release (%s) is available. Consider updating to latest version." (modify the fmt.Sprintf(...) return in the function that builds the release/update message).cli/commander/reference.go (1)
16-38:⚠️ Potential issue | 🟡 Minor
--plainflag is now a no-op.
isPlainis declared, registered as a flag, and passed intorunReferenceCommand, but the handler body (Line 35-37) never reads it. Since the markdown/ANSI rendering path was removed, the flag has no effect yet remains in the CLI surface—users who pass--plain=falsewill silently get the same plain output. Either drop the flag and the pointer plumbing, or restore the ANSI rendering branch.🔧 Proposed cleanup (drop the dead flag)
func (m *Manager) addReferenceCommand() { - var isPlain bool refCmd := &cobra.Command{ Use: "reference", Short: "Comprehensive reference of all commands", Long: m.generateReferenceMarkdown(), - Run: m.runReferenceCommand(&isPlain), + Run: m.runReferenceCommand(), Annotations: map[string]string{ "group": "help", }, } - refCmd.SetHelpFunc(m.runReferenceCommand(&isPlain)) - refCmd.Flags().BoolVarP(&isPlain, "plain", "p", true, "output in plain markdown (without ANSI color)") + refCmd.SetHelpFunc(m.runReferenceCommand()) m.RootCmd.AddCommand(refCmd) } -func (m *Manager) runReferenceCommand(isPlain *bool) func(cmd *cobra.Command, args []string) { +func (m *Manager) runReferenceCommand() func(cmd *cobra.Command, args []string) { return func(cmd *cobra.Command, _ []string) { fmt.Print(cmd.Long) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commander/reference.go` around lines 16 - 38, The --plain flag is dead: remove the unused isPlain variable, the BoolVarP registration (refCmd.Flags().BoolVarP(&isPlain, "plain", "p", true, ...)), and the pointer parameter plumbing from runReferenceCommand (change func (m *Manager) runReferenceCommand(isPlain *bool) to func (m *Manager) runReferenceCommand() and update calls refCmd.Run and refCmd.SetHelpFunc to m.runReferenceCommand()); keep the body that prints cmd.Long (fmt.Print(cmd.Long)). This removes the no-op flag and simplifies Manager.runReferenceCommand and its usages.
🟡 Minor comments (18)
README.md-68-70 (1)
68-70:⚠️ Potential issue | 🟡 MinorAdd a language to the installation code fence.
This fixes the markdownlint
MD040warning.Markdown lint fix
-``` +```sh go get github.com/raystack/salt</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 68 - 70, The code block containing the command "go
get github.com/raystack/salt" in README.md is missing a language tag causing an
MD040 warning; update the triple-backtick fence to include a shell language
identifier (e.g.,sh orbash) immediately after the opening backticks so
the block reads as a shell snippet and satisfies markdownlint.</details> </blockquote></details> <details> <summary>MIGRATION.md-202-205 (1)</summary><blockquote> `202-205`: _⚠️ Potential issue_ | _🟡 Minor_ **Update migration note to include error handling for `defaults.Set`.** The current migration text omits that `defaults.Set` returns an error. Without error handling, invalid default tags would be silently ignored, which is a behavioral change from the old library. <details> <summary>Proposed migration text</summary> ```diff -// API change: defaults.SetDefaults(cfg) → defaults.Set(cfg) +// API change: defaults.SetDefaults(cfg) → if err := defaults.Set(cfg); err != nil { return err } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@MIGRATION.md` around lines 202 - 205, The migration note must mention that defaults.Set now returns an error and instruct users to handle it: update the MIGRATION.md snippet replacing the current line referencing defaults.Set(cfg) with text explaining that callers should check the returned error from defaults.Set (e.g., propagate or log/handle invalid default tag errors), and mention that behavior differs from the old defaults.SetDefaults which did not return errors; reference the function name defaults.Set in the note so readers know to add error handling where they previously called defaults.SetDefaults. ``` </details> </blockquote></details> <details> <summary>config/config_test.go-147-148 (1)</summary><blockquote> `147-148`: _⚠️ Potential issue_ | _🟡 Minor_ **Add error handling for `defaults.Set` call in test setup.** `defaults.Set` returns an error; ignoring it can hide broken test setup. The test should fail fast if defaults cannot be applied, consistent with the error handling pattern used for `loader.Load` on line 154. <details> <summary>Fail fast when defaults cannot be applied</summary> ```diff // Apply defaults - defaults.Set(cfg) + if err := defaults.Set(cfg); err != nil { + t.Fatalf("Failed to apply defaults: %v", err) + } cfg.Server.Port = 3000 // Default value ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@config/config_test.go` around lines 147 - 148, The test currently ignores the error returned by defaults.Set(cfg); update the test setup to check the error and fail fast if it is non-nil—mirror the pattern used for loader.Load by capturing the error from defaults.Set, calling t.Fatalf (or require.NoError) with a clear message if it fails, and keep the rest of the setup unchanged; this ensures failures applying defaults to cfg are surfaced immediately. ``` </details> </blockquote></details> <details> <summary>middleware/requestlog/requestlog.go-79-104 (1)</summary><blockquote> `79-104`: _⚠️ Potential issue_ | _🟡 Minor_ **HTTP middleware logs all requests at Info, even 5xx.** Unlike the Connect interceptor (which differentiates `Error` vs `Info` based on handler error), the HTTP path always calls `cfg.logger.Info` regardless of the captured status. Consider bumping to `Error` for `status >= 500` so server errors aren't hidden under Info in production log aggregators. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@middleware/requestlog/requestlog.go` around lines 79 - 104, The HTTPMiddleware currently always logs via cfg.logger.Info even when the response indicates server errors; update the handler inside HTTPMiddleware (around statusWriter usage) to check sw.status and call cfg.logger.Error when sw.status >= 500 (keeping the same structured fields: "method", r.Method, "path", path, "status", sw.status, "duration", duration.String(), "request_id", rid), otherwise continue to call cfg.logger.Info; ensure you reference the statusWriter instance (sw) and the logger on cfg so behavior mirrors the Connect interceptor's error-level logging for 5xx responses. ``` </details> </blockquote></details> <details> <summary>.github/workflows/lint.yaml-27-29 (1)</summary><blockquote> `27-29`: _⚠️ Potential issue_ | _🟡 Minor_ **Use a specific patch version of golangci-lint (e.g., `v2.11.0`).** The tag `v2.11` does not exist; release tags are `v2.11.0` through `v2.11.4`. The `golangci-lint-action@v7` correctly supports `v2.x` per its release notes, but the version must be pinned to an actual release tag. Additionally, consider pinning the action to a commit SHA instead of a major version tag for improved supply-chain hygiene. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint.yaml around lines 27 - 29, The workflow currently sets golangci-lint version to the non-existent tag "v2.11" and uses the action by major tag "golangci/golangci-lint-action@v7"; update the "version:" value to a specific patch release (e.g., change the value from v2.11 to v2.11.0) and optionally pin the action reference (uses:) to a full commit SHA instead of `@v7` for stronger supply-chain hygiene so the lines referencing golangci/golangci-lint-action@v7 and version: v2.11 are replaced with the specific, existing release tag and/or a commit SHA. ``` </details> </blockquote></details> <details> <summary>cli/example_test.go-34-34 (1)</summary><blockquote> `34-34`: _⚠️ Potential issue_ | _🟡 Minor_ **Check `Execute` errors in examples.** Lines 34 and 53 discard command execution failures, so broken CLI wiring can go unnoticed in copied examples. <details> <summary>Suggested change</summary> ```diff - rootCmd.Execute() + if err := rootCmd.Execute(); err != nil { + panic(err) + } ``` ```diff - rootCmd.Execute() + if err := rootCmd.Execute(); err != nil { + panic(err) + } ``` </details> Also applies to: 53-53 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/example_test.go` at line 34, The example test currently discards errors from rootCmd.Execute(), so CLI wiring failures go unnoticed; change both calls (the rootCmd.Execute() at lines referenced) to capture the returned error and fail the test if non-nil (e.g. if err := rootCmd.Execute(); err != nil { t.Fatalf("rootCmd.Execute() failed: %v", err) }). Ensure you update both occurrences so examples properly surface execution errors. ``` </details> </blockquote></details> <details> <summary>server/server_test.go-22-42 (1)</summary><blockquote> `22-42`: _⚠️ Potential issue_ | _🟡 Minor_ **Remove hard-coded ports and fixed startup sleeps.** These tests can fail when the chosen ports are already in use or the server takes longer than 100ms to accept requests. Capture `srv.Start(ctx)` errors and poll the endpoint until it is ready. <details> <summary>Suggested test pattern</summary> ```diff - go srv.Start(ctx) - time.Sleep(100 * time.Millisecond) + errCh := make(chan error, 1) + go func() { errCh <- srv.Start(ctx) }() + + require.Eventually(t, func() bool { + resp, err := http.Get("http://127.0.0.1:18923/ping") + if err != nil { + return false + } + resp.Body.Close() + return resp.StatusCode == http.StatusOK + }, 2*time.Second, 25*time.Millisecond) ``` </details> Also applies to: 74-89, 118-131, 138-151 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/server_test.go` around lines 22 - 42, Replace the hard-coded port and fixed sleep by letting the OS pick an available port and by capturing server startup errors and polling the /ping endpoint until it responds; specifically, update tests that call server.New (and server.WithAddr("127.0.0.1:18923")) to use an ephemeral address (":0"), start srv with srv.Start(ctx) but capture any returned error from Start (or run Start in a goroutine and send errors on a channel), then poll http.Get("http://<resolved-addr>/ping") with a short retry loop and timeout instead of time.Sleep(100*time.Millisecond) to wait for readiness before asserting response fields. Ensure the changes are applied to the blocks using server.New, server.WithAddr, srv.Start and any direct http.Get calls referenced at the noted ranges. ``` </details> </blockquote></details> <details> <summary>app/app_test.go-48-70 (1)</summary><blockquote> `48-70`: _⚠️ Potential issue_ | _🟡 Minor_ **Avoid fixed ports and sleep-based readiness in lifecycle tests.** These subtests bind hard-coded ports and rely on `time.Sleep(100 * time.Millisecond)`. That can flake in CI due to port conflicts or slow startup, and several `Start` errors are ignored. Prefer dynamically allocated ports plus a readiness poll, and capture the `Start` result through an error channel. <details> <summary>Suggested test pattern</summary> ```diff - go a.Start(ctx) - time.Sleep(100 * time.Millisecond) + errCh := make(chan error, 1) + go func() { errCh <- a.Start(ctx) }() + + require.Eventually(t, func() bool { + resp, err := http.Get("http://127.0.0.1:18953/hello") + if err != nil { + return false + } + resp.Body.Close() + return resp.StatusCode == http.StatusOK + }, 2*time.Second, 25*time.Millisecond) ``` </details> Also applies to: 79-94, 101-116, 123-142, 156-175 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/app_test.go` around lines 48 - 70, The test uses a hard-coded port via app.WithAddr("127.0.0.1:18950") and a time.Sleep to wait for readiness, which can flake; change to bind to a dynamic port (create a net.Listener on "127.0.0.1:0" and pass its Addr to app.New or construct the address from listener.Addr()) and remove the fixed sleep; start the server in a goroutine capturing a.Start(ctx) into errCh (keep errCh) and implement a readiness poll that repeatedly GETs "/ping" with short intervals and an overall timeout (e.g., loop until http.Get returns 200 or timeout) instead of sleeping; ensure any Start error is read from errCh and asserted after cancel to avoid ignored errors (references: app.New, app.WithAddr, a.Start, errCh, "/ping"). ``` </details> </blockquote></details> <details> <summary>cli/cli_test.go-66-75 (1)</summary><blockquote> `66-75`: _⚠️ Potential issue_ | _🟡 Minor_ **Assert the version output.** This test captures `root` output but never inspects `buf`, so it would pass even if the `version` command printed nothing. <details> <summary>Suggested change</summary> ```diff err := root.Execute() require.NoError(t, err) + assert.Contains(t, buf.String(), "2.5.0") }) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/cli_test.go` around lines 66 - 75, The test "version command prints version" currently captures output into buf but never asserts it; update the test to read and assert the buffer contents after executing root.Execute(): use buf.String() (or strings.Contains) to verify the output includes the expected version "2.5.0" (or the exact expected version string/format produced by cli.Version) so the test fails if the version command prints nothing; locate this logic around newTestRoot(), cli.Init(...) and root.SetOut(&buf) in the test and add the assertion using require.Contains or require.Equal on buf.String(). ``` </details> </blockquote></details> <details> <summary>cli/config_cmd.go-21-25 (1)</summary><blockquote> `21-25`: _⚠️ Potential issue_ | _🟡 Minor_ **Annotate the top-level `config` command for help grouping.** Only the subcommands are marked as `core`; the root-level `config` command itself has no group annotation, so it can be omitted from the intended grouped layout. <details> <summary>Suggested change</summary> ```diff cmd := &cobra.Command{ - Use: "config <command>", - Short: "Manage client configuration", - Example: fmt.Sprintf(" $ %s config init\n $ %s config list", appName, appName), + Use: "config <command>", + Short: "Manage client configuration", + Example: fmt.Sprintf(" $ %s config init\n $ %s config list", appName, appName), + Annotations: map[string]string{ + "group": "core", + }, } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/config_cmd.go` around lines 21 - 25, The top-level cobra command instance named cmd (type *cobra.Command) is missing the group annotation so it won't appear in the "core" help grouping; set the command's GroupID to "core" (e.g., assign cmd.GroupID = "core" after constructing the &cobra.Command) so the root-level "config" command is included alongside its subcommands in the grouped help output. ``` </details> </blockquote></details> <details> <summary>server/server_test.go-45-63 (1)</summary><blockquote> `45-63`: _⚠️ Potential issue_ | _🟡 Minor_ **Actually exercise h2c in the h2c test.** This subtest uses `http.Get`, which only proves HTTP/1.1 still works. Use an h2c-capable HTTP/2 client and assert `resp.ProtoMajor == 2` to verify HTTP/2 cleartext is actually negotiated. <details> <summary>Suggested direction</summary> ```go client := &http.Client{ Transport: &http2.Transport{ AllowHTTP: true, DialTLSContext: func(ctx context.Context, network, addr string, _ *tls.Config) (net.Conn, error) { return (&net.Dialer{}).DialContext(ctx, network, addr) }, }, } resp, err := client.Get("http://127.0.0.1:18924/ping") require.NoError(t, err) defer resp.Body.Close() assert.Equal(t, 2, resp.ProtoMajor) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/server_test.go` around lines 45 - 63, The subtest "h2c enabled by default" currently uses http.Get (HTTP/1.1); replace that call with an h2c-capable client: construct an http.Client whose Transport is an http2.Transport with AllowHTTP: true and DialTLSContext implemented via net.Dialer, then use that client to GET "http://127.0.0.1:18924/ping" (instead of http.Get) and assert resp.ProtoMajor == 2 to verify HTTP/2 cleartext negotiation; update imports to include the http2 package (golang.org/x/net/http2) as needed and keep the rest of the test using server.New and srv.Start as-is. ``` </details> </blockquote></details> <details> <summary>middleware/cors/cors.go-42-113 (1)</summary><blockquote> `42-113`: _⚠️ Potential issue_ | _🟡 Minor_ **Permissive-by-default CORS — document or reconsider.** `Defaults()` allows all origins (`"*"`), and `DefaultHTTP` composes these defaults unless callers override them. Two things worth noting: 1. `isOriginAllowed` returns true for `"*"`, so the middleware echoes the specific request `Origin` back in `Access-Control-Allow-Origin` rather than sending the literal `"*"`. Combined with any future `Access-Control-Allow-Credentials: true`, this would silently become an unsafe "allow any origin with credentials" configuration. Since credentials aren't currently configurable this is fine today, but please call it out in doc/config if credentials support is added later. 2. Consider documenting (in the package doc comment and/or README/MIGRATION) that `middleware.DefaultHTTP` opens CORS to every origin by default, so service authors know to pass `cors.WithAllowedOrigins(...)` in production. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@middleware/cors/cors.go` around lines 42 - 113, Defaults() currently allows all origins and isOriginAllowed treats "*" as allowed but middleware echoes the request Origin back, which is risky if credentials are ever enabled; update the package docs and the DefaultHTTP usage comment to explicitly state that Defaults() is permissive and recommend callers pass cors.WithAllowedOrigins(...) for production, and change the middleware logic around isOriginAllowed/Defaults: either remove "*" from Defaults() or modify Middleware to detect a literal "*" in cfg.allowedOrigins and in that case set Access-Control-Allow-Origin to "*" (instead of echoing the request Origin) so wildcard handling won’t silently enable "allow any origin with credentials" behavior; reference Defaults(), isOriginAllowed, Middleware and DefaultHTTP when making these changes. ``` </details> </blockquote></details> <details> <summary>app/app.go-76-100 (1)</summary><blockquote> `76-100`: _⚠️ Potential issue_ | _🟡 Minor_ **Shutdown path has no bound on `onStop` hooks, and `server.WithLogger` silently overrides user options.** Two small concerns in the shutdown/build flow: 1. `a.stop(context.Background())` (line 84) passes a context with no deadline, so any `onStop` hook that hangs (e.g., a stuck DB flush or external-service call) blocks shutdown forever. Since the server already has a grace period, consider deriving a timeout for hook execution — either reusing a configurable `app` grace period or per-hook. 2. `server.WithLogger(a.logger)` is appended after user `serverOps` (line 78), so it silently overrides a caller-supplied `server.WithLogger(...)`. If this is intentional (app owns logging), worth a brief comment/godoc note so users aren't surprised. <details> <summary>🛠 Suggested timeout for onStop hooks</summary> ```diff - err := srv.Start(ctx) - - // Shutdown sequence. - a.stop(context.Background()) - return err + err := srv.Start(ctx) + + // Shutdown sequence with a bounded timeout so hung hooks can't block forever. + stopCtx, cancel := context.WithTimeout(context.Background(), a.stopTimeout()) + defer cancel() + a.stop(stopCtx) + return err ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 76 - 100, The shutdown path should bound onStop hooks and avoid silently clobbering user-provided logger options: change a.stop to accept the server context and inside it derive a cancellable timeout for hooks (e.g., ctxWithTimeout, cancel := context.WithTimeout(ctx, a.gracePeriod) or a configurable per-hook timeout) and use that ctxWithTimeout when calling each fn(ctx), ensuring cancel() is deferred and logging a timeout error if a hook returns ctx.DeadlineExceeded; for the server logger, don't unconditionally append server.WithLogger(a.logger) after a.serverOps — either document that the App owns/overrides the logger or change the logic to only append server.WithLogger when no user server.Option supplies a logger (detect existing server.WithLogger in a.serverOps) so callers' options are not silently overridden. ``` </details> </blockquote></details> <details> <summary>server/server.go-89-101 (1)</summary><blockquote> `89-101`: _⚠️ Potential issue_ | _🟡 Minor_ **Edge case: closed `errCh` yields `fmt.Errorf("server serve: %w", nil)`.** If `srv.Serve` returns `http.ErrServerClosed` *before* `ctx.Done()` fires (e.g., if `srv.Close`/`srv.Shutdown` is ever invoked by another path, or in a future refactor), the goroutine skips the write and closes the channel. The `case err := <-errCh` branch then receives the zero value (`nil`) and the function returns a non-nil error wrapping nil — confusing for callers. Distinguish "channel closed cleanly" from "error received": <details> <summary>🛠 Suggested guard</summary> ```diff select { - case err := <-errCh: - return fmt.Errorf("server serve: %w", err) + case err, ok := <-errCh: + if ok { + return fmt.Errorf("server serve: %w", err) + } + // errCh closed without an error: fall through to shutdown. case <-ctx.Done(): } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/server.go` around lines 89 - 101, The goroutine closing errCh can make the select receive a nil and cause fmt.Errorf("server serve: %w", nil); fix by distinguishing a closed channel from an actual error: when reading from errCh in the select, use the comma-ok receive (err, ok := <-errCh) and only return a wrapped error if ok && err != nil; if !ok (channel closed) treat it as clean shutdown and continue/return nil (or proceed to ctx.Done handling). Update logic around errCh, the goroutine that calls srv.Serve, and the select to implement this check. ``` </details> </blockquote></details> <details> <summary>cli/cli.go-50-60 (1)</summary><blockquote> `50-60`: _⚠️ Potential issue_ | _🟡 Minor_ **Wrap both `PersistentPreRun` and `PersistentPreRunE` to ensure context injection isn't bypassed.** Cobra selects `PersistentPreRunE` over `PersistentPreRun` when both exist on the same command, and subcommands completely override the parent's hook when they define their own (unless `EnableTraverseRunHooks` is enabled). This implementation only wraps `PersistentPreRun`, leaving two paths where context injection is skipped: 1. If a caller sets `rootCmd.PersistentPreRunE` (common for error-returning init functions), the wrapper on `PersistentPreRun` is never invoked. 2. If a subcommand defines its own `PersistentPreRun` or `PersistentPreRunE`, it bypasses the root's wrapper for that branch. In both cases, `Output(cmd)` and `Prompter(cmd)` silently fall back to new instances (lines 102, 112), functionally working but defeating shared state — test-injected output/prompter won't reach those code paths. Add wrapping for `PersistentPreRunE` and document the requirement for subcommands, or use a mechanism independent of pre-run hooks (e.g., always fetch context-aware instances rather than relying on injection). <details> <summary>Suggested fix: wrap both variants</summary> ```diff - existing := rootCmd.PersistentPreRun - rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) { - ctx := context.WithValue(cmd.Context(), contextKey{}, &cliContext{ - output: printer.NewOutput(os.Stdout), - prompter: prompt.New(), - }) - cmd.SetContext(ctx) - if existing != nil { - existing(cmd, args) - } - } + inject := func(cmd *cobra.Command) { + ctx := context.WithValue(cmd.Context(), contextKey{}, &cliContext{ + output: printer.NewOutput(os.Stdout), + prompter: prompt.New(), + }) + cmd.SetContext(ctx) + } + switch { + case rootCmd.PersistentPreRunE != nil: + existingE := rootCmd.PersistentPreRunE + rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + inject(cmd) + return existingE(cmd, args) + } + default: + existing := rootCmd.PersistentPreRun + rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) { + inject(cmd) + if existing != nil { + existing(cmd, args) + } + } + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/cli.go` around lines 50 - 60, The current wrapper only replaces rootCmd.PersistentPreRun, so context injection is skipped when callers set rootCmd.PersistentPreRunE or when subcommands override hooks; update the code to also wrap rootCmd.PersistentPreRunE by saving the original into a variable (e.g., existingE) and assigning a wrapper that builds and sets the context (using contextKey{} and cliContext with printer.NewOutput/prompter.New) then calls existingE(cmd, args) and returns its error, while preserving the existing PersistentPreRun behavior; also add a brief comment near Output(cmd)/Prompter(cmd) noting subcommands must not override the root hooks (or consider switching to always-fetch context-aware instances) so test-injected output/prompter are not bypassed. ``` </details> </blockquote></details> <details> <summary>cli/prompt/prompt.go-40-47 (1)</summary><blockquote> `40-47`: _⚠️ Potential issue_ | _🟡 Minor_ **`Select`: silent fallback to index 0 when `defaultValue` does not match any option.** If the caller passes a `defaultValue` that is not in `options` (or passes `""` when no option is empty), `result` silently remains `0`, which means the first option is pre-selected without any indication the default was invalid. Consider either requiring a match, skipping the pre-selection when there is no match, or documenting this fallback explicitly. <details> <summary>♻️ Suggested behavior: only pre-select when defaultValue matches</summary> ```diff - var result int - // Find default index - for i, opt := range options { - if opt == defaultValue { - result = i - break - } - } + result := -1 + for i, opt := range options { + if opt == defaultValue { + result = i + break + } + } + // If defaultValue didn't match, let huh start with no pre-selection. + if result < 0 { + result = 0 // or leave as-is if huh tolerates out-of-range; document either way + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/prompt/prompt.go` around lines 40 - 47, The Select logic currently leaves result at 0 when defaultValue isn't found, silently pre-selecting the first option; modify the loop in Select to track whether a match was found (e.g., found bool) and only assign result when opt == defaultValue, and after the loop handle the "no match" case by either setting result to -1 (or nil-equivalent) so no pre-selection is made or by returning an error/report; update any code that consumes result to accept the no-preselect sentinel instead of assuming index 0. Ensure you reference the Select function and the variables options, defaultValue, and result when making the change. ``` </details> </blockquote></details> <details> <summary>cli/prompt/prompt.go-97-111 (1)</summary><blockquote> `97-111`: _⚠️ Potential issue_ | _🟡 Minor_ **`Input`: `defaultValue` is wired as a placeholder, creating semantic ambiguity.** 1. `Placeholder` in Huh displays hint text only when the input field is empty—it is not a pre-filled editable value. Users expecting the default to be already typed in and overridable will not see that behavior. 2. The `if result == "" { result = defaultValue }` fallback on lines 107–109 combined with placeholder-only semantics means callers cannot distinguish between "user deliberately submitted empty" and "user accepted the default". If `defaultValue` is non-empty, empty submissions become impossible from this API. To provide a true default value: initialize `result = defaultValue` before `.Run()` (Huh will show it as editable text) and remove the post-run rewrite. To allow empty submissions with a hint: drop the post-run fallback. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/prompt/prompt.go` around lines 97 - 111, The Input method currently uses Placeholder(defaultValue) and then overwrites an empty result after Run, which conflates hints with true editable defaults; update huhPrompter.Input by initializing result = defaultValue before calling huh.NewInput().Title(...).Value(&result)...Run() so the default appears as editable text, remove the post-Run fallback block (the if result == "" { result = defaultValue }), and keep or remove Placeholder(defaultValue) as desired (but do not rely on post-run rewriting) to preserve distinction between an explicit empty submission and accepting the default. ``` </details> </blockquote></details> <details> <summary>cli/printer/printer.go-195-202 (1)</summary><blockquote> `195-202`: _⚠️ Potential issue_ | _🟡 Minor_ **`Progress` is not gated on `o.tty`, unlike `Spin`.** `Spin` returns a no-op `Indicator{}` when `!o.tty` (lines 181-184), but `Progress` always returns a live `progressbar` bound to `o.w`. If `w` is not a terminal (e.g., piped output, CI logs), the progress bar will emit control characters and repeated redraws. For consistency and clean non-TTY output, gate the progress bar on `o.tty` by either short-circuiting (similar to `Spin`) or passing `progressbar.OptionSetVisibility(o.tty)` to suppress rendering in non-TTY environments. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 195 - 202, The Progress method currently always creates a live progressbar bound to o.w even when o.tty is false; mirror Spin's behavior by short-circuiting when !o.tty (returning a no-op Indicator/compatible stub) or add progressbar.OptionSetVisibility(o.tty) to the NewOptions call so the bar doesn't render/control-char when not a TTY; update Output.Progress (and ensure return type remains compatible with Indicator/consumer code) to check o.tty and either return the no-op Indicator used by Spin or construct the progressbar with OptionSetVisibility(o.tty). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (7)</summary><blockquote> <details> <summary>server/example_test.go (1)</summary><blockquote> `13-40`: **Minor: discarded `Start` return value and port reuse between examples.** Same polish note as `app/example_test.go`: if `srv.Start(ctx)` returns an `error`, both examples drop it (Line 25, Line 39). Showing the error check in godoc makes the example more honest. Also, both examples bind to `:8080`. Since examples without `// Output:` aren't executed by `go test` this is harmless today, but if an `// Output:` directive is ever added, the two will race for the port. Consider `:0` (or different ports) to future-proof. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/example_test.go` around lines 13 - 40, Example functions ExampleNew and ExampleNew_withTimeouts discard the return value of srv.Start(ctx) and both bind to port ":8080"; capture and handle the error returned by server.Start (e.g., err := srv.Start(ctx); if err != nil { fmt.Println(err) } or log it) and change the WithAddr calls to use ":0" (or distinct ports) to avoid future port conflicts when examples are run; update the two example functions (ExampleNew, ExampleNew_withTimeouts) to perform this error check and use the new addr. ``` </details> </blockquote></details> <details> <summary>cli/prompt/example_test.go (1)</summary><blockquote> `9-32`: **Example will block on interactive input during `go test`.** Without an `// Output:` directive, `go test` compiles this example but does not execute it, so today it is effectively only godoc. If that's the intent, fine. But if you ever add an `// Output:` line (or someone runs `go test -run ExampleNew` expecting it to execute), the example will hang waiting for TTY input and `panic` on error, which is a poor doc pattern. Consider either (a) leaving a short comment in the file noting the example is illustrative and not executed, or (b) returning errors instead of `panic` so the example reads more like production guidance. Non-blocking. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/prompt/example_test.go` around lines 9 - 32, The ExampleNew example blocks on interactive TTY input and currently panics on errors (p.Select, p.Input, p.Confirm), so update it to avoid hanging during `go test`: either add a short top-of-function comment like "Illustrative only — not executed by tests; interactive" to document it won't be run, or refactor the example to handle errors instead of panicking (replace the panic(err) calls with simple error handling that prints the error and returns from ExampleNew) so the example becomes non-blocking and safe if executed. ``` </details> </blockquote></details> <details> <summary>app/example_test.go (1)</summary><blockquote> `13-37`: **Add error handling to both examples; `ExampleNew` lacks `// Output:` and won't execute as a test.** Two polish items for the godoc examples: 1. `ExampleRun` (lines 14–21) ignores the error from `app.Run()`, and `ExampleNew` (line 36) ignores the error from `a.Start(ctx)`. Both functions return `error` per the app package API, so ignoring it teaches incorrect practice. Examples should demonstrate proper error handling. 2. `ExampleNew` has no `// Output:` comment, so the example code compiles but won't execute under `go test`. This means the `:8080` port bind won't actually occur, masking a potential runtime issue. Adding the error check (below) keeps the example sound if someone later adds `// Output:` to enable execution. <details> <summary>🔧 Suggested fix</summary> ```diff func ExampleNew() { a, err := app.New( app.WithLogger(slog.Default()), app.WithAddr(":8080"), ) if err != nil { panic(err) } ctx, cancel := context.WithCancel(context.Background()) defer cancel() - a.Start(ctx) + if err := a.Start(ctx); err != nil { + panic(err) + } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/example_test.go` around lines 13 - 37, Both examples currently ignore returned errors; update ExampleRun to capture and handle the error from app.Run(...) (e.g., check err and call panic or log.Fatalf) and update ExampleNew to check the error returned by a.Start(ctx) (handle similarly) so callers see correct error handling, and add a // Output: line to ExampleNew so the example is executed by go test; locate the calls to app.Run in ExampleRun and app.New / a.Start in ExampleNew and insert the error checks and the // Output: comment. ``` </details> </blockquote></details> <details> <summary>.golangci.yml (1)</summary><blockquote> `1-21`: **Add `goimports.local-prefixes` configuration for proper import grouping.** The v2 configuration is correctly structured. However, to enable proper import grouping in goimports, add: ``` formatters: settings: goimports: local-prefixes: - github.com/raystack/salt ``` Without this, goimports will only separate stdlib from third-party imports, not group your local imports. Also, confirm that disabling `errcheck` is intentional—the codebase has significant error handling in server and telemetry packages that would benefit from error checking, so this should not be a migration oversight. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 1 - 21, Add goimports local-prefixes under the existing formatters block so goimports groups local imports correctly: add a formatters.settings.goimports.local-prefixes entry with your module prefix (e.g., github.com/raystack/salt) so local imports are separated from third-party ones; also revisit the linters.disable.errcheck entry (linters.disable.errcheck) and either remove it or document/confirm it's intentional because server and telemetry packages rely on error handling and would benefit from enabling errcheck. ``` </details> </blockquote></details> <details> <summary>cli/cli_test.go (1)</summary><blockquote> `79-95`: **Make the context-backed output assertion observable.** This test currently sets `out = &bytes.Buffer{}` independently of `cli.Output(cmd)`, so it would pass even if `cli.Output` always returned its fallback. At minimum, assert `Execute` succeeds and track that the command path ran; ideally, assert behavior that differs between context output and fallback output. <details> <summary>Suggested tightening</summary> ```diff - var out *bytes.Buffer + var ran bool child := &cobra.Command{ Use: "child", Run: func(cmd *cobra.Command, _ []string) { o := cli.Output(cmd) assert.NotNil(t, o) - out = &bytes.Buffer{} + ran = true }, } root.AddCommand(child) root.SetArgs([]string{"child"}) - root.Execute() - assert.NotNil(t, out) + require.NoError(t, root.Execute()) + assert.True(t, ran) }) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/cli_test.go` around lines 79 - 95, The test's assertion is ineffective because it sets out independently instead of using the context-backed output; change the child command's Run to obtain and use the context output returned by cli.Output(cmd) (assign out = o.(*bytes.Buffer) or write to that buffer) so the test observes what cli.Output provides, assert that root.Execute() returns nil (succeeds), and then assert on out's contents or that out is the same instance returned by cli.Output to verify context vs fallback behavior; reference the test's use of cli.Init(root), cli.Output(cmd) and root.Execute() to locate and update the code. ``` </details> </blockquote></details> <details> <summary>cli/printer/printer.go (2)</summary><blockquote> `207-248`: **Recompute-per-call: every `Green`/`Yellow`/... rebuilds the theme.** Each of these helpers calls `newTheme()`, which in turn calls `termenv.EnvColorProfile()` (which does env inspection and isatty checks) and `termenv.HasDarkBackground()` (which may do a terminal round-trip on some platforms). For code that formats many styled strings (tables, logs, progress messages), this is measurable overhead and also makes the color choice non-deterministic if the environment is queried at different moments. Cache the profile/theme once at package init (`var defaultTheme = newTheme()`), and have these helpers reuse it. <details> <summary>♻️ Proposed refactor</summary> ```diff +var defaultTheme = newTheme() + // Green returns text styled in green. -func Green(t string) string { return colorize(newTheme().Green, t) } +func Green(t string) string { return colorize(defaultTheme.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(defaultTheme.Yellow, t) } // ... (apply to Cyan/Red/Grey/Blue/Magenta similarly) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 207 - 248, The helpers (Green, Yellow, Cyan, Red, Grey, Blue, Magenta and their formatted variants Greenf/Yellowf/.../Magentaf) repeatedly call newTheme(), which triggers expensive environment/terminal queries each call; instead create a package-level cached theme (e.g. var defaultTheme = newTheme()) at init and update all helpers to use defaultTheme.Green / defaultTheme.Yellow / etc. for coloring and have the formatted helpers call their corresponding non-formatted helper (e.g. Greenf uses Green(fmt.Sprintf(...))) so no per-call newTheme() is invoked. ``` </details> --- `127-164`: **`Markdown` and `MarkdownWithWrap` are near-duplicates — consider collapsing.** The two functions differ only by the argument to `glamour.WithWordWrap`. `Markdown(text)` is equivalent to `MarkdownWithWrap(text, 0)` today. Worth deduping to a single private renderer to avoid drift (e.g. if the style JSON or options list changes in one but not the other). <details> <summary>♻️ Proposed refactor</summary> ```diff func (o *Output) Markdown(text string) error { - text = strings.ReplaceAll(text, "\r\n", "\n") - tr, err := glamour.NewTermRenderer( - glamour.WithAutoStyle(), - glamour.WithEmoji(), - glamour.WithWordWrap(0), - glamour.WithStylesFromJSONBytes([]byte(`{"document":{"margin":0},"code_block":{"margin":0}}`)), - ) - if err != nil { - return err - } - rendered, err := tr.Render(text) - if err != nil { - return err - } - fmt.Fprint(o.w, rendered) - return nil + return o.MarkdownWithWrap(text, 0) } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 127 - 164, Markdown and MarkdownWithWrap are duplicate; consolidate by factoring renderer creation into a single helper and have Markdown call the wrap-aware implementation (or have both call a private method). Update Output.Markdown to call Output.MarkdownWithWrap(text, 0) or create a private method like renderMarkdown(text string, wrap int) that constructs the glamour.TermRenderer (glamour.NewTermRenderer with glamour.WithWordWrap(wrap) and the shared WithStylesFromJSONBytes option) and performs tr.Render and fmt.Fprint; replace duplicated logic in both Markdown and MarkdownWithWrap to use that helper so style/option changes stay in one place. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `8c9d0245-33a2-48e0-8924-9a9d5f66f8a4` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b213c3509e1e87f6bf27e923ef0605f551f2992e and ae9d123d14bad3de2fb5a4c9581e1de8be03841e. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (100)</summary> * `.github/workflows/lint.yaml` * `.golangci.yml` * `MIGRATION.md` * `README.md` * `app/app.go` * `app/app_test.go` * `app/example_test.go` * `app/option.go` * `auth/audit/audit.go` * `auth/audit/audit_test.go` * `auth/audit/mocks/repository.go` * `auth/audit/model.go` * `auth/audit/repositories/dockertest_test.go` * `auth/audit/repositories/postgres.go` * `auth/audit/repositories/postgres_test.go` * `auth/oidc/_example/main.go` * `auth/oidc/cobra.go` * `auth/oidc/redirect.html` * `auth/oidc/source_gsa.go` * `auth/oidc/source_oidc.go` * `auth/oidc/utils.go` * `cli/cli.go` * `cli/cli_test.go` * `cli/commander/completion.go` * `cli/commander/layout.go` * `cli/commander/reference.go` * `cli/config_cmd.go` * `cli/example_test.go` * `cli/option.go` * `cli/printer/colors.go` * `cli/printer/example_test.go` * `cli/printer/markdown.go` * `cli/printer/printer.go` * `cli/printer/progress.go` * `cli/printer/spinner.go` * `cli/printer/structured.go` * `cli/printer/table.go` * `cli/printer/text.go` * `cli/prompt/example_test.go` * `cli/prompt/prompt.go` * `cli/prompter/prompt.go` * `cli/terminal/brew.go` * `cli/terminal/browser.go` * `cli/terminal/pager.go` * `cli/terminal/term.go` * `cli/version/release.go` * `config/config.go` * `config/config_test.go` * `config/example_test.go` * `db/config.go` * `db/db.go` * `db/db_test.go` * `db/migrate.go` * `db/migrate_test.go` * `db/migrations/1481574547_create_users_table.down.sql` * `db/migrations/1481574547_create_users_table.up.sql` * `go.mod` * `middleware/cors/cors.go` * `middleware/errorz/errorz.go` * `middleware/example_test.go` * `middleware/middleware.go` * `middleware/middleware_test.go` * `middleware/recovery/recovery.go` * `middleware/requestid/requestid.go` * `middleware/requestlog/requestlog.go` * `observability/logger/logger.go` * `observability/logger/logrus.go` * `observability/logger/logrus_test.go` * `observability/logger/noop.go` * `observability/logger/zap.go` * `observability/logger/zap_test.go` * `observability/otelgrpc/otelgrpc.go` * `observability/otelgrpc/otelgrpc_test.go` * `observability/otelgrpc/status.go` * `observability/otelgrpc/status_test.go` * `observability/otelhttpclient/annotations.go` * `observability/otelhttpclient/http_transport.go` * `observability/otelhttpclient/http_transport_test.go` * `server/example_test.go` * `server/mux/README.md` * `server/mux/mux.go` * `server/mux/option.go` * `server/mux/serve_target.go` * `server/option.go` * `server/server.go` * `server/server_test.go` * `server/spa/handler.go` * `telemetry/opentelemetry.go` * `telemetry/telemetry.go` * `testing/dockertestx/README.md` * `testing/dockertestx/configs/cortex/single_process_cortex.yaml` * `testing/dockertestx/configs/nginx/cortex_nginx.conf` * `testing/dockertestx/cortex.go` * `testing/dockertestx/dockertestx.go` * `testing/dockertestx/minio.go` * `testing/dockertestx/minio_migrate.go` * `testing/dockertestx/nginx.go` * `testing/dockertestx/postgres.go` * `testing/dockertestx/spicedb.go` * `testing/dockertestx/spicedb_migrate.go` </details> <details> <summary>💤 Files with no reviewable changes (56)</summary> * auth/oidc/redirect.html * db/migrations/1481574547_create_users_table.up.sql * db/migrations/1481574547_create_users_table.down.sql * cli/printer/table.go * db/config.go * auth/oidc/cobra.go * cli/printer/progress.go * auth/oidc/_example/main.go * auth/oidc/source_gsa.go * auth/audit/repositories/postgres_test.go * cli/printer/markdown.go * auth/audit/audit_test.go * testing/dockertestx/README.md * cli/prompter/prompt.go * auth/audit/repositories/dockertest_test.go * auth/audit/mocks/repository.go * auth/audit/model.go * cli/printer/structured.go * db/db_test.go * observability/otelhttpclient/http_transport_test.go * server/mux/serve_target.go * observability/logger/logger.go * server/mux/README.md * observability/otelgrpc/otelgrpc_test.go * observability/logger/logrus_test.go * auth/oidc/utils.go * db/migrate_test.go * testing/dockertestx/configs/cortex/single_process_cortex.yaml * db/migrate.go * server/mux/mux.go * testing/dockertestx/configs/nginx/cortex_nginx.conf * observability/otelhttpclient/annotations.go * auth/audit/repositories/postgres.go * observability/otelgrpc/otelgrpc.go * observability/logger/noop.go * db/db.go * server/mux/option.go * observability/logger/zap_test.go * auth/audit/audit.go * testing/dockertestx/dockertestx.go * observability/otelgrpc/status_test.go * observability/logger/zap.go * observability/otelgrpc/status.go * cli/printer/text.go * cli/printer/spinner.go * testing/dockertestx/postgres.go * observability/otelhttpclient/http_transport.go * observability/logger/logrus.go * auth/oidc/source_oidc.go * testing/dockertestx/nginx.go * testing/dockertestx/minio.go * testing/dockertestx/spicedb.go * testing/dockertestx/spicedb_migrate.go * testing/dockertestx/cortex.go * testing/dockertestx/minio_migrate.go * cli/printer/colors.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
ravisuhag
force-pushed
the
refactor/salt-evolution
branch
3 times, most recently
from
2736a86 to
8e1a499
Compare
Commander layout now reads both cobra's native GroupID and the legacy Annotations["group"] for command grouping. Projects can use either approach while keeping salt's custom help formatting. cli.Init sets SetErrPrefix(name + ":") for consistent error messages across all raystack CLIs (e.g. "Error: frontier: unknown command").
ravisuhag
force-pushed
the
refactor/salt-evolution
branch
from
8e1a499 to
02cd603
Compare
Only CheckForUpdate is the public API. FetchInfo, CompareVersions, Info, Timeout, APIFormat were exported but only used internally. Reduces the public API surface.
Add error types for CLI commands:
- ErrSilent: command already printed error, exit 1 silently
- ErrCancel: user cancelled (ctrl-c), exit 0
- FlagError: bad flags/args, print error + usage
- HandleError(err): helper that handles all types
Remove commander.IsCommandErr which used fragile string matching.
Usage:
if err := rootCmd.Execute(); err != nil {
cli.HandleError(err)
}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cli/commander/layout.go (1)
118-120: Make grouped help sections deterministic.Iterating
groupCommandsdirectly randomizes section order when multiple command groups exist, which can make help output and golden tests flaky. Sort group IDs before appending sections.♻️ Proposed fix
+ groupNames := make([]string, 0, len(groupCommands)) + for group := range groupCommands { + groupNames = append(groupNames, group) + } + sort.Strings(groupNames) - for group, cmds := range groupCommands { + for _, group := range groupNames { + cmds := groupCommands[group] helpEntries = append(helpEntries, helpEntry{fmt.Sprintf("%s commands", toTitle(group)), strings.Join(cmds, "\n")}) }Add the import:
import ( "bytes" "errors" "fmt" "regexp" + "sort" "strings"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commander/layout.go` around lines 118 - 120, The help generation iterates map groupCommands directly causing nondeterministic section order; to fix, collect the map keys into a slice, sort that slice (e.g., using sort.Strings), then iterate the sorted keys when appending to helpEntries (refer to groupCommands, helpEntries, helpEntry, and toTitle) and add the "sort" import to the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cli.go`:
- Around line 49-60: The Init currently replaces only rootCmd.PersistentPreRun
and therefore bypasses any existing rootCmd.PersistentPreRunE hooks; update Init
to wrap and chain both rootCmd.PersistentPreRun and rootCmd.PersistentPreRunE so
existing hooks run in the original order and errors from PersistentPreRunE are
returned. Specifically, for symbols rootCmd, PersistentPreRun, and
PersistentPreRunE, capture existing hooks (existingPreRun and existingPreRunE),
set new wrappers that inject the cliContext into cmd.Context(), call the
existingPreRun first if present, then call existingPreRunE (returning its error)
or vice-versa to preserve Cobra’s execution order, and ensure any error returned
by the wrapped PersistentPreRunE is propagated up instead of being ignored.
In `@MIGRATION.md`:
- Around line 9-11: The fenced code block in MIGRATION.md lacks a language tag,
triggering MD040; update the block opening from ``` to ```go.mod so the snippet
becomes fenced as go.mod (i.e., change the fence that currently surrounds "go
1.24" to use the language identifier "go.mod") to keep the migration guide
lint-clean.
- Around line 224-228: Update the migration note to state that the replacement
call defaults.Set(cfg) (from "github.com/creasty/defaults") returns an error and
therefore cannot be used as a drop-in replacement for defaults.SetDefaults(cfg)
from "github.com/mcuadros/go-defaults"; change the text to explicitly mention
the new signature and show that callers must handle its error return (e.g., call
defaults.Set(cfg) and check the returned error, logging or returning it as
appropriate), and include a short prose example that indicates checking the
error and handling failure so readers know to update code that previously
ignored return values.
---
Nitpick comments:
In `@cli/commander/layout.go`:
- Around line 118-120: The help generation iterates map groupCommands directly
causing nondeterministic section order; to fix, collect the map keys into a
slice, sort that slice (e.g., using sort.Strings), then iterate the sorted keys
when appending to helpEntries (refer to groupCommands, helpEntries, helpEntry,
and toTitle) and add the "sort" import to the file.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51af4363-b7a4-4743-8f31-cb25425b635a
📒 Files selected for processing (10)
MIGRATION.mdcli/cli.gocli/commander/codex.gocli/commander/layout.gocli/commander/manager.gocli/commander/reference.gocli/commander/topics.gocli/config_cmd.gocli/prompt/prompt.gocli/version/release.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/commander/reference.go
- cli/prompt/prompt.go
Version checking: - Results cached for 24h at ~/.config/raystack/<repo>/state.json - Eliminates HTTP call to GitHub on every command invocation - Cache respects XDG_CONFIG_HOME and APPDATA on Windows Table output: - TTY: tab-aligned columns with padding (human-friendly) - Non-TTY: raw tab-separated values (pipe to awk/cut/jq)
Terminal: - Add Width() returning terminal column count (defaults to 80) Printer: - Status output (Success, Warning, Error, Info, Spin, Progress) now writes to stderr instead of stdout - Data output (Table, JSON, YAML, Println, Print) still writes to stdout - This prevents spinners and status messages from corrupting piped data: myapp list --json | jq '.name' # spinner goes to stderr, JSON to stdout
Add cli.Execute() that owns all error dispatch (sentinels, flag errors,
default). Silence Cobra's built-in error/usage printing so Execute has
full control. Unexport FlagError and remove HandleError/NewFlagError —
flag wrapping is now internal to Init via SetFlagErrorFunc.
Modernize across packages: interface{} → any, os.IsNotExist →
errors.Is(fs.ErrNotExist), sort.Slice → slices.SortFunc. Update
migration guide and README to document cli.Execute.
- Remove -h shorthand on host flag to avoid panic from Cobra's built-in help flag conflict (cli/example_test.go) - Handle error from defaults.Set instead of ignoring it (config.go) - Preserve PersistentPreRunE in addition to PersistentPreRun so Init doesn't silently drop user hooks (cli.go) - Add Unwrap() to statusWriter so http.Flusher/Hijacker pass through, fixing streaming and h2c support (requestlog.go)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
cli/version/release.go (3)
22-25:tarURLis dead field.
releaseInfo.tarURLis populated infetchInfo(line 68) but never read anywhere —CheckForUpdateonly usesinfo.version. Either drop it, or surface it in the update message (e.g., a download link) if that was the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/version/release.go` around lines 22 - 25, The struct field releaseInfo.tarURL is assigned in fetchInfo but never used; either remove tarURL from releaseInfo and stop populating it in fetchInfo, or propagate it into the update path by returning/using it in CheckForUpdate (where info.version is currently used) and include the URL in the update message (e.g., show a download link). Locate the releaseInfo type, the fetchInfo function that sets tarURL, and the CheckForUpdate code path that reads info.version and update the implementation to either delete tarURL and its assignments or add code to read info.tarURL and surface it in the update notification.
157-168: Consider usingos.UserConfigDir()from the stdlib.
os.UserConfigDir()already handlesXDG_CONFIG_HOME,APPDATAon Windows, and falls back appropriately on macOS (~/Library/Application Support) and Linux (~/.config). The current implementation silently ignores the macOS convention and theos.UserHomeDir()error.♻️ Proposed refactor
-func configDir() string { - if dir := os.Getenv("XDG_CONFIG_HOME"); dir != "" { - return dir - } - if runtime.GOOS == "windows" { - if dir := os.Getenv("APPDATA"); dir != "" { - return dir - } - } - home, _ := os.UserHomeDir() - return filepath.Join(home, ".config") -} +func configDir() string { + dir, err := os.UserConfigDir() + if err != nil { + return "" + } + return dir +}Note that this changes the macOS location to the platform-idiomatic
~/Library/Application Support— if the doc comment onCheckForUpdatepromising~/.config/raystack/<repo>/state.jsonis the intended contract across all OSes, keep the current impl and at least drop the unusedruntimeimport branch concern by returning an error signal when home lookup fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/version/release.go` around lines 157 - 168, The configDir() function currently reimplements platform logic, ignores os.UserHomeDir() errors and omits macOS conventions; replace its body to use the standard library os.UserConfigDir() (or, if you must preserve the explicit "~/.config/raystack/..." contract mentioned in CheckForUpdate's doc comment, keep the current path but at minimum handle errors from os.UserHomeDir() by returning or propagating an error and remove the unused runtime import branch); update callers of configDir() as needed to accept an error return if you choose to propagate errors, and ensure CheckForUpdate's documentation is reconciled with the chosen behavior.
72-84: Function namecompareVersionsobscures its semantics.The function returns
currentVersion >= latestVersion, i.e., whether the current version is already up-to-date, but the name suggests a generic comparison. The caller at line 113 names the resultisLatest, which is clearer. Consider renaming toisUpToDate(current, latest string) (bool, error)so that readers ofcompareVersionsdon't have to inspect the implementation to infer direction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/version/release.go` around lines 72 - 84, Rename the function compareVersions to isUpToDate and keep its signature isUpToDate(current, latest string) (bool, error); update all callers to use isUpToDate (the caller that currently assigns the result to isLatest should call isUpToDate) so the API expresses the semantics (returns true when current >= latest) without changing behavior, and update any related comments/tests to reflect the new name.config/config.go (1)
134-134: Reuse a single*validator.Validateinstance.
validator.New()builds and caches struct metadata per instance; constructing a fresh one on everyLoad()call throws that cache away. For a CLI this is negligible, but for services callingLoad()repeatedly (hot-reload, tests) it's wasteful. Consider hoisting a package-level (orLoader-level)var v = validator.New()and reusing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` at line 134, Replace the per-call validator.New() in the Load() validation line with a reused validator instance: declare a package-level var (e.g. var v = validator.New()) or a Loader-level field and call v.Struct(config) instead of validator.New().Struct(config); update any tests/initialization to ensure the shared validator is constructed once and used on subsequent Load() calls to avoid rebuilding the struct metadata cache each time.cli/printer/printer.go (2)
141-178:MarkdownandMarkdownWithWrapare near-duplicates.They differ only in the
glamour.WithWordWrapargument. DRY it up:♻️ Proposed refactor
-func (o *Output) Markdown(text string) error { - text = strings.ReplaceAll(text, "\r\n", "\n") - tr, err := glamour.NewTermRenderer( - glamour.WithAutoStyle(), - glamour.WithEmoji(), - glamour.WithWordWrap(0), - glamour.WithStylesFromJSONBytes([]byte(`{"document":{"margin":0},"code_block":{"margin":0}}`)), - ) - if err != nil { - return err - } - rendered, err := tr.Render(text) - if err != nil { - return err - } - fmt.Fprint(o.w, rendered) - return nil -} - -// MarkdownWithWrap renders markdown with a specified word wrap width. -func (o *Output) MarkdownWithWrap(text string, wrap int) error { +func (o *Output) Markdown(text string) error { + return o.MarkdownWithWrap(text, 0) +} + +// MarkdownWithWrap renders markdown with a specified word wrap width. +func (o *Output) MarkdownWithWrap(text string, wrap int) error { text = strings.ReplaceAll(text, "\r\n", "\n") ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 141 - 178, The Markdown and MarkdownWithWrap methods on Output are duplicated except for the glamour.WithWordWrap value; refactor by extracting a single helper (e.g., renderMarkdown or renderMarkdownWithWrap) that accepts the text and wrap int and performs the shared logic (normalize CRLF, create glamour.TermRenderer with glamour.WithWordWrap(wrap) and the styles, render, and write to o.w), then make both Markdown call that helper (Markdown passes wrap=0 or a default, MarkdownWithWrap passes the provided wrap) to remove duplication; update or remove the original duplicated code in Markdown and MarkdownWithWrap accordingly.
286-302:newTheme()is recomputed on every package-level color helper call.
Green,Yellow,Cyan,Red,Grey,Blue,Magentaeach callnewTheme(), which in turn callstermenv.EnvColorProfile()andtermenv.HasDarkBackground(). The latter may perform a blocking terminal query (per termenv docs) and both re-read process environment. Cache once at package init:♻️ Proposed refactor
-func Green(t string) string { return colorize(newTheme().Green, t) } +var defaultTheme = newTheme() + +func Green(t string) string { return colorize(defaultTheme.Green, t) } ...Combine with gating on TTY if these helpers remain in the public API. (Also tracks the duplicate TTY-leak concern above.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 286 - 302, newTheme() is being recomputed on every color helper call (Green, Yellow, Cyan, Red, Grey, Blue, Magenta), causing repeated termenv.EnvColorProfile() and termenv.HasDarkBackground() invocations; change this by computing the Theme once at package initialization and returning the cached Theme from the helper functions: introduce a package-level variable (e.g., cachedTheme) and initialize it in an init() or via sync.Once in newThemeInit(), populate Green/Yellow/Cyan/Red/Grey/Blue/Magenta from that cached Theme, and optionally gate initialization behind a TTY check if these helpers are public to avoid blocking terminal queries during non-interactive runs.cli/config_cmd.go (1)
40-42: Surface a hint whenloader.Initfails with "already exists".
Initreturns the sentinelerrors.New("configuration file already exists")(config/config.go:147). Returning it raw fromRunEcauses Cobra to print the error plus the usage block, which is noisy for what is effectively a benign state. Consider catching this specific case and printing a friendlier status viaOutput(cmd).Warning(...)(and optionally returningcli.ErrSilent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/config_cmd.go` around lines 40 - 42, When RunE calls loader.Init(defaultCfg) and it returns the sentinel error with message "configuration file already exists", catch that specific case instead of returning it raw: check if err != nil && err.Error() == "configuration file already exists", call Output(cmd).Warning("configuration file already exists") (or a friendlier message), and return cli.ErrSilent; otherwise return the original err. This change should be applied around the RunE invocation that calls loader.Init(defaultCfg) so RunE, loader.Init, Output(cmd).Warning and cli.ErrSilent are the referenced symbols to update.cli/cli.go (1)
47-53: Remove the unusedSetErrPrefixcall.When
SilenceErrors = true(line 52), Cobra does not use the configured error prefix for its own output. TheExecutefunction (lines 96-117) handles all error printing with its own formatting: line 109 prints the error directly (for flag errors), and line 114 uses a hardcoded"Error: "prefix (for other errors). TheSetErrPrefixcall at line 48 is never invoked and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cli.go` around lines 47 - 53, Remove the unused call to rootCmd.SetErrPrefix(...) since Cobra's SilenceErrors is enabled and Execute handles error formatting itself; delete the SetErrPrefix invocation (the call on rootCmd) and keep the SilenceErrors and SilenceUsage assignments and existing Execute error handling intact (refer to rootCmd, SetErrPrefix, SilenceErrors, SilenceUsage, and Execute to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cli.go`:
- Around line 79-83: The comment above rootCmd.SetFlagErrorFunc is incorrect
about commander.Manager.Init() setting a flag error func; update that comment to
remove the ordering claim and instead state simply that
rootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { return
&flagError{err: err} }) wraps flag parsing errors so Execute can show contextual
usage. Reference the symbols rootCmd.SetFlagErrorFunc and
commander.Manager.Init() in the updated comment to clarify that Init() does not
modify flag error handling.
In `@cli/commander/layout.go`:
- Around line 107-109: The help generation currently iterates the map
groupCommands which yields non-deterministic ordering; replace that map
iteration with a deterministic loop over cmd.Groups() (the order established by
cmd.AddGroup()) and for each group key use groupCommands[group] when building
helpEntries (helpEntry, toTitle remain unchanged) so the "... commands" sections
appear in registration order.
In `@cli/config_cmd.go`:
- Around line 54-61: The command is calling loader.View() which returns
viper.AllSettings() without having read the config file (SetConfigFile is set by
WithAppConfig but v.ReadInConfig() is never invoked), so add a public method
Read() on the config loader that calls the underlying v.ReadInConfig()
(analogous to what Load() does but without requiring a struct), and then update
the cobra RunE to call loader.Read() before calling loader.View(); reference
config.NewLoader, config.WithAppConfig, loader.Read(), loader.View(), and
loader.Load() when making the changes.
In `@cli/version/release.go`:
- Line 116: Fix the minor grammar in the release message string: update the
fmt.Sprintf return that uses 'latest' so the sentence capitalizes "Consider" and
includes the article "the" before "latest version" (i.e., change "A new release
(%s) is available. consider updating to latest version." to "A new release (%s)
is available. Consider updating to the latest version."). Locate the return
statement that formats the message (the fmt.Sprintf call referencing latest) and
replace the string accordingly.
---
Nitpick comments:
In `@cli/cli.go`:
- Around line 47-53: Remove the unused call to rootCmd.SetErrPrefix(...) since
Cobra's SilenceErrors is enabled and Execute handles error formatting itself;
delete the SetErrPrefix invocation (the call on rootCmd) and keep the
SilenceErrors and SilenceUsage assignments and existing Execute error handling
intact (refer to rootCmd, SetErrPrefix, SilenceErrors, SilenceUsage, and Execute
to locate the code).
In `@cli/config_cmd.go`:
- Around line 40-42: When RunE calls loader.Init(defaultCfg) and it returns the
sentinel error with message "configuration file already exists", catch that
specific case instead of returning it raw: check if err != nil && err.Error() ==
"configuration file already exists", call Output(cmd).Warning("configuration
file already exists") (or a friendlier message), and return cli.ErrSilent;
otherwise return the original err. This change should be applied around the RunE
invocation that calls loader.Init(defaultCfg) so RunE, loader.Init,
Output(cmd).Warning and cli.ErrSilent are the referenced symbols to update.
In `@cli/printer/printer.go`:
- Around line 141-178: The Markdown and MarkdownWithWrap methods on Output are
duplicated except for the glamour.WithWordWrap value; refactor by extracting a
single helper (e.g., renderMarkdown or renderMarkdownWithWrap) that accepts the
text and wrap int and performs the shared logic (normalize CRLF, create
glamour.TermRenderer with glamour.WithWordWrap(wrap) and the styles, render, and
write to o.w), then make both Markdown call that helper (Markdown passes wrap=0
or a default, MarkdownWithWrap passes the provided wrap) to remove duplication;
update or remove the original duplicated code in Markdown and MarkdownWithWrap
accordingly.
- Around line 286-302: newTheme() is being recomputed on every color helper call
(Green, Yellow, Cyan, Red, Grey, Blue, Magenta), causing repeated
termenv.EnvColorProfile() and termenv.HasDarkBackground() invocations; change
this by computing the Theme once at package initialization and returning the
cached Theme from the helper functions: introduce a package-level variable
(e.g., cachedTheme) and initialize it in an init() or via sync.Once in
newThemeInit(), populate Green/Yellow/Cyan/Red/Grey/Blue/Magenta from that
cached Theme, and optionally gate initialization behind a TTY check if these
helpers are public to avoid blocking terminal queries during non-interactive
runs.
In `@cli/version/release.go`:
- Around line 22-25: The struct field releaseInfo.tarURL is assigned in
fetchInfo but never used; either remove tarURL from releaseInfo and stop
populating it in fetchInfo, or propagate it into the update path by
returning/using it in CheckForUpdate (where info.version is currently used) and
include the URL in the update message (e.g., show a download link). Locate the
releaseInfo type, the fetchInfo function that sets tarURL, and the
CheckForUpdate code path that reads info.version and update the implementation
to either delete tarURL and its assignments or add code to read info.tarURL and
surface it in the update notification.
- Around line 157-168: The configDir() function currently reimplements platform
logic, ignores os.UserHomeDir() errors and omits macOS conventions; replace its
body to use the standard library os.UserConfigDir() (or, if you must preserve
the explicit "~/.config/raystack/..." contract mentioned in CheckForUpdate's doc
comment, keep the current path but at minimum handle errors from
os.UserHomeDir() by returning or propagating an error and remove the unused
runtime import branch); update callers of configDir() as needed to accept an
error return if you choose to propagate errors, and ensure CheckForUpdate's
documentation is reconciled with the chosen behavior.
- Around line 72-84: Rename the function compareVersions to isUpToDate and keep
its signature isUpToDate(current, latest string) (bool, error); update all
callers to use isUpToDate (the caller that currently assigns the result to
isLatest should call isUpToDate) so the API expresses the semantics (returns
true when current >= latest) without changing behavior, and update any related
comments/tests to reflect the new name.
In `@config/config.go`:
- Line 134: Replace the per-call validator.New() in the Load() validation line
with a reused validator instance: declare a package-level var (e.g. var v =
validator.New()) or a Loader-level field and call v.Struct(config) instead of
validator.New().Struct(config); update any tests/initialization to ensure the
shared validator is constructed once and used on subsequent Load() calls to
avoid rebuilding the struct metadata cache each time.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c74860f7-7bf5-4adb-9f1a-8032000a72ed
📒 Files selected for processing (26)
MIGRATION.mdREADME.mdapp/option.gocli/cli.gocli/cli_test.gocli/commander/codex.gocli/commander/completion.gocli/commander/layout.gocli/commander/manager.gocli/config_cmd.gocli/error.gocli/example_test.gocli/printer/example_test.gocli/printer/printer.gocli/terminal/term.gocli/version/release.goconfig/config.goconfig/helpers.godata/jsondiff/README.mddata/jsondiff/jsondiff.godata/jsondiff/jsondiff_test.godata/jsondiff/jsondiff_wi2l.godata/jsondiff/jsondiff_wi2l_test.godata/jsondiff/reconstructor.godata/rql/parser.gogo.mod
✅ Files skipped from review due to trivial changes (11)
- data/jsondiff/jsondiff_wi2l_test.go
- data/jsondiff/jsondiff_test.go
- data/jsondiff/README.md
- config/helpers.go
- data/rql/parser.go
- data/jsondiff/jsondiff_wi2l.go
- data/jsondiff/reconstructor.go
- cli/printer/example_test.go
- README.md
- cli/cli_test.go
- MIGRATION.md
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/commander/completion.go
- cli/terminal/term.go
- cli/commander/manager.go
- cli/example_test.go
- app/option.go
Introduce IOStreams as the single owner of stdin/stdout/stderr with per-stream TTY detection, color awareness, pager integration, and prompt safety (CanPrompt). - System() for production, Test() for deterministic buffer-backed tests - cli.IO(cmd) extracts IOStreams from context; Output(cmd) and Prompter(cmd) remain as convenience shortcuts (unchanged API) - printer.NewOutputFrom(w, errW, tty) for explicit stream construction - SetStdoutTTY, SetColorEnabled, SetNeverPrompt for test overrides - StartPager/StopPager on IOStreams replaces direct terminal.Pager use
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
README.md (1)
68-70:⚠️ Potential issue | 🟡 MinorAdd language tag to fenced code block.
The installation code block is missing a language specification, which reduces clarity for readers and causes linting warnings.
📝 Proposed fix
-``` +```bash go get github.com/raystack/salt</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 68 - 70, Update the fenced code block containing the
command "go get github.com/raystack/salt" to include a language tag by changing
the opening backticks to ```bash so the block becomes a bash-marked code block;
this will resolve lint warnings and improve readability in README.md.</details> </blockquote></details> <details> <summary>cli/cli.go (1)</summary><blockquote> `95-99`: _⚠️ Potential issue_ | _🟡 Minor_ **Stale/incorrect comment — `mgr.Init()` does not set a flag error func.** This was flagged in a prior review and remains unchanged. `commander.Manager.Init()` (see `cli/commander/manager.go:63-75` in relevant snippets) only wires help groups, help/reference/completion/docs. It never calls `SetFlagErrorFunc`, so the "Must be set after mgr.Init()" ordering constraint is fictitious and will mislead future readers (e.g., someone may believe reordering this block changes behavior). <details> <summary>📝 Proposed fix</summary> ```diff - // Wrap flag parsing errors so Execute can show contextual usage. - // Must be set after mgr.Init() which also configures a flag error func. + // Wrap flag parsing errors in *flagError so Execute can show contextual usage. rootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { return &flagError{err: err} }) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/cli.go` around lines 95 - 99, The comment above rootCmd.SetFlagErrorFunc is incorrect about ordering with commander.Manager.Init; update the comment to accurately describe intent: state that we wrap Cobra flag parsing errors so Execute can show contextual usage and that this SetFlagErrorFunc must be set on the root command (rootCmd.SetFlagErrorFunc) to return a flagError type, without claiming it depends on commander.Manager.Init(); reference the functions rootCmd.SetFlagErrorFunc and commander.Manager.Init in the comment to clarify that Init configures help/docs only and does not set a flag error func. ``` </details> </blockquote></details> <details> <summary>cli/printer/printer.go (1)</summary><blockquote> `61-83`: _⚠️ Potential issue_ | _🟠 Major_ **ANSI escape sequences still leak to non-TTY writers.** The previous review on this point was marked resolved, but the current code continues to emit ANSI unconditionally: - `o.color()` at lines 310–312 does not check `o.tty` before delegating to `colorize`, so `Success`/`Warning`/`Error`/`Info` (lines 61–78) always embed escape codes into `o.errW`. - `Bold` (line 82) calls `termenv.String(msg).Bold().String()` with no TTY guard. - `Italic` (line 271) and all package-level color helpers `Green`/`Yellow`/`Cyan`/`Red`/`Grey`/`Blue`/`Magenta` and their `…f` variants (lines 227–268) are inherently writer-unaware and unconditionally emit escapes. Practical impact: a `bytes.Buffer` passed to `NewOutput(w)` (or `NewOutputFrom(w, errW, false)` with `tty=false`) still receives colorized stderr, breaking golden-file tests, log redirection, and any consumer that diff-compares captured output. It also means `NewOutputFrom(..., false)` silently lies — callers explicitly pass `tty=false` expecting plain text. Minimum fix: <details> <summary>🎨 Proposed fix</summary> ```diff func (o *Output) Bold(msg string) { - fmt.Fprintln(o.w, termenv.String(msg).Bold().String()) + if o.tty { + fmt.Fprintln(o.w, termenv.String(msg).Bold().String()) + return + } + fmt.Fprintln(o.w, msg) } @@ func (o *Output) color(c termenv.Color, t string) string { + if !o.tty { + return t + } return colorize(c, t) } ``` </details> The package-level helpers (`Green`, `Italic`, etc.) cannot know their writer and are structurally incompatible with TTY-aware output — consider either documenting them as always-colorized or removing them in favor of `*Output` methods. Also applies to: 310-315 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 61 - 83, The output methods are unconditionally emitting ANSI escapes; update the TTY handling so color is only applied when o.tty is true: modify Output.color(...) to return the raw msg when o.tty==false and ensure Output.Success, Warning, Error, Info use that guarded color path; change Output.Bold and Italic to respect o.tty (either call the guarded color/format helper or early‑return plain text when o.tty==false); and address package-level helpers (Green/Yellow/Cyan/Red/Grey/Blue/Magenta and their …f variants) by either removing/deprecating them or documenting they always emit escapes and replacing callers with Output methods (created via NewOutput or NewOutputFrom) so tests that pass tty=false get plain text. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>cli/cli.go (1)</summary><blockquote> `59-65`: **`SetErrPrefix` has no observable effect under `SilenceErrors = true`.** With `SilenceErrors = true` (line 64), Cobra suppresses its own error printing entirely, and `Execute` (lines 112–133) does all stderr writing itself — neither the flag-error path (line 125) nor the default path (line 130) uses `rootCmd.ErrPrefix()`. The `SetErrPrefix(rootCmd.Name() + ":")` call is therefore dead configuration and somewhat misleading (it reads as if errors will be prefixed with the binary name). Either drop it, or have `Execute` honor the prefix: <details> <summary>🧹 Option A: drop the dead call</summary> ```diff - // Set error prefix for consistent error messages. - rootCmd.SetErrPrefix(rootCmd.Name() + ":") - // Silence cobra's default error and usage printing. // Errors are handled by Execute; usage is shown only for flag errors. rootCmd.SilenceErrors = true rootCmd.SilenceUsage = true ``` </details> <details> <summary>🧹 Option B: use the prefix in Execute</summary> ```diff - fmt.Fprintf(os.Stderr, "Error: %s\n", err) + fmt.Fprintf(os.Stderr, "%s %s\n", rootCmd.ErrPrefix(), err) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cli/cli.go` around lines 59 - 65, The call to rootCmd.SetErrPrefix(rootCmd.Name() + ":") is dead because SilenceErrors = true prevents Cobra from printing errors with that prefix; either remove the SetErrPrefix call or make Execute use rootCmd.ErrPrefix() when writing errors. Fix by deleting the SetErrPrefix(...) invocation or, if you prefer keeping a prefix, update the Execute implementation (the function that calls rootCmd.Execute() and later writes to stderr in the flag-error and default error paths) to prepend rootCmd.ErrPrefix() to the messages it writes (use rootCmd.ErrPrefix() when constructing the error output in the flag-error branch and the general error branch). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@cli/iostreams.go:
- Around line 118-143: The StopPager method fails to restore IOStreams.Out and
invalidate the cached Output, causing writes to hit a closed pager pipe; add an
origOut io.Writer field to IOStreams, set s.origOut = s.Out in StartPager before
assigning s.Out = p.Out, and in StopPager restore s.Out = s.origOut, set
s.origOut = nil, invalidate s.output = nil, and clear s.pager and s.pagerStarted
after calling s.pager.Stop() so subsequent writes and Output() use the original,
live writer.
Duplicate comments:
In@cli/cli.go:
- Around line 95-99: The comment above rootCmd.SetFlagErrorFunc is incorrect
about ordering with commander.Manager.Init; update the comment to accurately
describe intent: state that we wrap Cobra flag parsing errors so Execute can
show contextual usage and that this SetFlagErrorFunc must be set on the root
command (rootCmd.SetFlagErrorFunc) to return a flagError type, without claiming
it depends on commander.Manager.Init(); reference the functions
rootCmd.SetFlagErrorFunc and commander.Manager.Init in the comment to clarify
that Init configures help/docs only and does not set a flag error func.In
@cli/printer/printer.go:
- Around line 61-83: The output methods are unconditionally emitting ANSI
escapes; update the TTY handling so color is only applied when o.tty is true:
modify Output.color(...) to return the raw msg when o.tty==false and ensure
Output.Success, Warning, Error, Info use that guarded color path; change
Output.Bold and Italic to respect o.tty (either call the guarded color/format
helper or early‑return plain text when o.tty==false); and address package-level
helpers (Green/Yellow/Cyan/Red/Grey/Blue/Magenta and their …f variants) by
either removing/deprecating them or documenting they always emit escapes and
replacing callers with Output methods (created via NewOutput or NewOutputFrom)
so tests that pass tty=false get plain text.In
@README.md:
- Around line 68-70: Update the fenced code block containing the command "go get
github.com/raystack/salt" to include a language tag by changing the opening
backticks to ```bash so the block becomes a bash-marked code block; this will
resolve lint warnings and improve readability in README.md.
Nitpick comments:
In@cli/cli.go:
- Around line 59-65: The call to rootCmd.SetErrPrefix(rootCmd.Name() + ":") is
dead because SilenceErrors = true prevents Cobra from printing errors with that
prefix; either remove the SetErrPrefix call or make Execute use
rootCmd.ErrPrefix() when writing errors. Fix by deleting the SetErrPrefix(...)
invocation or, if you prefer keeping a prefix, update the Execute implementation
(the function that calls rootCmd.Execute() and later writes to stderr in the
flag-error and default error paths) to prepend rootCmd.ErrPrefix() to the
messages it writes (use rootCmd.ErrPrefix() when constructing the error output
in the flag-error branch and the general error branch).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `11fd7386-5b52-4edf-b68c-60adcaf8ac75` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8e152b6ad792cac978e3fa2306531aacb42b8a91 and 4837a4f651a098903b7b7347f9c2e3059cbc28d6. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `MIGRATION.md` * `README.md` * `cli/cli.go` * `cli/example_test.go` * `cli/iostreams.go` * `cli/iostreams_test.go` * `cli/printer/printer.go` * `config/config.go` * `middleware/requestlog/requestlog.go` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * MIGRATION.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * middleware/requestlog/requestlog.go * config/config.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Remove IsTTY, IsColorDisabled, and Width from the terminal package — these are now per-stream methods on IOStreams (IsStdoutTTY, ColorEnabled, TerminalWidth). OpenBrowser uses isatty directly instead of IsTTY. The terminal package retains OS-level utilities: IsCI, OpenBrowser, Pager, IsUnderHomebrew, HasHomebrew.
Add Exporter interface and AddJSONFlags helper that gives any command a --json flag with comma-separated field selection, validation, and shell completion. - Fields extracted via json struct tags with case-insensitive fallback - Exportable interface for custom field handling - StructExportData helper for simple structs - Pretty-printed JSON on TTY, compact when piped - Rejects unknown fields with helpful error listing available fields
- 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
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
MIGRATION.md (1)
119-121:⚠️ Potential issue | 🟠 MajorDon’t reintroduce Cobra’s reserved
-hshorthand in the migration guide.Line 120 still documents
-hforhost; Cobra reserves-hfor help, so readers copying this example can hit the same duplicate-shorthand panic. Match the fixed example code by removing the shorthand or using a non-reserved one.Suggested fix
rootCmd := &cobra.Command{Use: "frontier", Short: "identity management"} -rootCmd.PersistentFlags().StringP("host", "h", "", "API host") +rootCmd.PersistentFlags().String("host", "", "API host") rootCmd.AddCommand(serverCmd, configCmd)#!/bin/bash # Verify no remaining examples use Cobra's reserved -h shorthand for host. rg -n -C2 'StringP\("host",\s*"h"' --glob '*.go' --glob '*.md'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MIGRATION.md` around lines 119 - 121, The migration guide shows reintroducing Cobra's reserved -h shorthand by calling PersistentFlags().StringP("host", "h", ... ) on rootCmd; update the example to remove the shorthand or pick a non-reserved shorthand (e.g., "H") so Cobra's help flag is not shadowed — change the StringP call for "host" to use String (no shorthand) or StringP with a safe letter, and keep rootCmd.AddCommand(serverCmd, configCmd) unchanged.cli/printer/printer.go (1)
39-43:⚠️ Potential issue | 🟠 MajorKeep styling/progress behind the correct stream TTY gate.
This unresolved TTY leak is still present:
o.color()andBold()style unconditionally, andProgress()always enables color codes. Since status output goes toerrW, a single stdout-derivedttyalso enables spinners/progress on redirected stderr or disables them when stdout is piped but stderr is interactive. Track output/error TTY separately and gate status styling/progress on the stderr TTY.Suggested direction
type Output struct { w io.Writer errW io.Writer theme Theme - tty bool + outTTY bool + errTTY bool }func (o *Output) Bold(msg string) { + if !o.errTTY { + fmt.Fprintln(o.errW, msg) + return + } fmt.Fprintln(o.errW, termenv.String(msg).Bold().String()) }func (o *Output) Progress(max int, description string) *progressbar.ProgressBar { return progressbar.NewOptions(max, progressbar.OptionSetWriter(o.errW), - progressbar.OptionEnableColorCodes(true), + progressbar.OptionEnableColorCodes(o.errTTY), progressbar.OptionSetDescription(description), progressbar.OptionShowCount(), ) }func (o *Output) color(c termenv.Color, t string) string { + if !o.errTTY { + return t + } return colorize(c, t) }#!/bin/bash # Verify the current implementation still uses one TTY bit and that style/progress paths bypass it. rg -n -C3 'tty\s+bool|NewOutputFrom|func \(o \*Output\) (Bold|Progress|color)\b|OptionEnableColorCodes\(true\)' cli/printer/printer.goAlso applies to: 58-60, 84-87, 218-225, 314-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/printer/printer.go` around lines 39 - 43, The Output struct currently uses a single tty bool causing styling/progress to follow stdout; change it to track separate TTYs (e.g., outTTY and errTTY or ttyOut and ttyErr) and initialize them in NewOutputFrom by detecting TTYs for w and errW separately; update methods color(), Bold(), and Progress() to consult the stderr TTY flag (errTTY) when deciding to emit ANSI styling/spinners/progress so status output sent to errW is gated by the stderr TTY, and adjust any OptionEnableColorCodes handling to respect the per-stream TTY flags.
🧹 Nitpick comments (1)
app/app_test.go (1)
68-86: Flaky timing and swallowedStarterrors across subtests.Several subtests rely on
time.Sleep(100ms)to wait for server readiness and discard the error froma.Start(ctx)(lines 108, 131, 149, 184). IfStartfails to bind (e.g., port taken due to thefreeAddrTOCTOU window) or errors during startup, the subsequenthttp.Getfails with a misleading connection error instead of surfacing the real cause.http.Getalso has no timeout, so a hung server could stall the test.Consider polling
/pingwith a bounded retry loop until it returns 200 (or timeout), and propagatingStarterrors via a channel so failed startups produce a clear test failure. Example pattern:errCh := make(chan error, 1) go func() { errCh <- a.Start(ctx) }() client := &http.Client{Timeout: time.Second} require.Eventually(t, func() bool { resp, err := client.Get("http://" + addr + "/ping") if err != nil { return false } resp.Body.Close() return resp.StatusCode == http.StatusOK }, 3*time.Second, 20*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app_test.go` around lines 68 - 86, Replace the blind sleep and unchecked http.Get with a bounded health-check loop that both propagates errors from a.Start(ctx) via the existing errCh and polls /ping using an http.Client with a Timeout until it returns 200 or times out; specifically, keep launching a.Start(ctx) in a goroutine that sends its error to errCh, create an http.Client{Timeout: ...}, then poll "http://"+addr+"/ping" in a retry loop (or use require.Eventually) for a few seconds, failing fast if errCh returns a non-nil error, and only proceed to cancel and read from errCh after the health check succeeds or the overall timeout elapses so startup errors from a.Start are surfaced instead of being swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/app_test.go`:
- Around line 196-217: Rename the t.Run subtest title to accurately reflect its
assertion: change the string passed to t.Run (currently "onStart failure returns
error and runs cleanup") to something like "onStart failure returns error and
skips onStop hooks" so the test name matches the expectation asserted in the
test body; locate the t.Run invocation wrapping the test case (the anonymous
func that sets cleanupRan, uses app.New and asserts cleanupRan is false) and
update only the descriptive string.
In `@cli/export.go`:
- Around line 201-225: The extractData method in jsonExporter must guard against
invalid reflect.Value before using it: add an early check in extractData (the
function named extractData on type jsonExporter) to return nil if v.IsValid() is
false so you never call v.Interface() on an invalid value (this fixes the
Write(nil) -> extractData(reflect.ValueOf(nil)) panic); keep the existing switch
logic for valid kinds and ensure invalid or nil values return nil so JSON
encodes them as null.
In `@cli/terminal/term.go`:
- Around line 14-18: The IsCI function treats any non-empty CI env value as
true, so values like "false" or "0" incorrectly indicate CI; update IsCI to read
os.Getenv("CI"), normalize (strings.TrimSpace + strings.ToLower) and treat it as
CI only if non-empty and not equal to "false" or "0" (keep existing checks for
BUILD_NUMBER and RUN_ID unchanged); reference the IsCI function and the "CI",
"BUILD_NUMBER", and "RUN_ID" environment variable checks when making this
change.
- Around line 25-29: The OpenBrowser function currently checks stdout TTY which
blocks browser launches when stdout is redirected; change OpenBrowser(goos, url
string) to accept an interactive stream (e.g., add a parameter like in io.Reader
or an OS file descriptor/streams struct) and use that stream's file descriptor
with isatty.IsTerminal / IsCygwinTerminal instead of os.Stdout.Fd(); update the
internal check in OpenBrowser and keep returning openBrowserCmd(goos, url), and
update all callers to pass os.Stdin (or the IOStreams.Context stdin) so
interactivity is determined by stdin rather than stdout.
In `@middleware/cors/cors.go`:
- Around line 85-104: The current middleware short-circuits every OPTIONS
request with an Origin regardless of whether it's a CORS preflight; update the
logic to only treat OPTIONS as preflight when the Access-Control-Request-Method
header is present: in the branch that checks
!isOriginAllowed(cfg.allowedOrigins, origin) replace the unconditional r.Method
== http.MethodOptions short-circuit with a check for
r.Header.Get("Access-Control-Request-Method") != "" and only then write
StatusForbidden, otherwise call next.ServeHTTP; likewise in the allowed-origin
branch ensure the r.Method == http.MethodOptions -> StatusNoContent behavior is
gated on the same Access-Control-Request-Method header so application-level
OPTIONS handlers run when that header is absent.
- Line 83: The Vary header is being overwritten by w.Header().Set("Vary",
"Origin"); instead read the existing value via w.Header().Get("Vary"), check
case-insensitively whether "Origin" is already present (split on commas, trim
spaces), and only append ", Origin" when missing, then write back with
w.Header().Set("Vary", newValue); locate this change where
w.Header().Set("Vary", "Origin") is called in the cors middleware and replace it
with the conditional-get/append logic so existing Vary entries are preserved.
- Around line 40-57: Update Defaults() in cors.go to align with ConnectRPC CORS
expectations: restrict the default allowed methods to only "GET", "POST",
"OPTIONS" by changing the WithAllowedMethods(...) call so extra methods
("PUT","DELETE","PATCH") are not included (developers can opt into them via
WithAllowedMethods), and add a WithExposedHeaders(...) entry exposing
"Grpc-Status", "Grpc-Message", and "Grpc-Status-Details-Bin" so browser gRPC-web
clients can read response status headers; keep existing WithAllowedHeaders and
WithMaxAge as-is.
In `@middleware/errorz/errorz.go`:
- Around line 30-39: The WithLogger option currently allows setting c.logger to
nil which can cause panics; keep the safe discard logger fallback by ensuring
newConfig enforces a non-nil logger after applying opts: in newConfig (and/or in
WithLogger) check if c.logger is nil after the opts loop and, if so, reset it to
slog.New(slog.DiscardHandler()) so that config.logger never ends up nil when
code later calls cfg.logger.Error(...).
---
Duplicate comments:
In `@cli/printer/printer.go`:
- Around line 39-43: The Output struct currently uses a single tty bool causing
styling/progress to follow stdout; change it to track separate TTYs (e.g.,
outTTY and errTTY or ttyOut and ttyErr) and initialize them in NewOutputFrom by
detecting TTYs for w and errW separately; update methods color(), Bold(), and
Progress() to consult the stderr TTY flag (errTTY) when deciding to emit ANSI
styling/spinners/progress so status output sent to errW is gated by the stderr
TTY, and adjust any OptionEnableColorCodes handling to respect the per-stream
TTY flags.
In `@MIGRATION.md`:
- Around line 119-121: The migration guide shows reintroducing Cobra's reserved
-h shorthand by calling PersistentFlags().StringP("host", "h", ... ) on rootCmd;
update the example to remove the shorthand or pick a non-reserved shorthand
(e.g., "H") so Cobra's help flag is not shadowed — change the StringP call for
"host" to use String (no shorthand) or StringP with a safe letter, and keep
rootCmd.AddCommand(serverCmd, configCmd) unchanged.
---
Nitpick comments:
In `@app/app_test.go`:
- Around line 68-86: Replace the blind sleep and unchecked http.Get with a
bounded health-check loop that both propagates errors from a.Start(ctx) via the
existing errCh and polls /ping using an http.Client with a Timeout until it
returns 200 or times out; specifically, keep launching a.Start(ctx) in a
goroutine that sends its error to errCh, create an http.Client{Timeout: ...},
then poll "http://"+addr+"/ping" in a retry loop (or use require.Eventually) for
a few seconds, failing fast if errCh returns a non-nil error, and only proceed
to cancel and read from errCh after the health check succeeds or the overall
timeout elapses so startup errors from a.Start are surfaced instead of being
swallowed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3920dc22-6268-4657-9cde-0beb0a55c82e
📒 Files selected for processing (19)
.github/workflows/lint.yamlMIGRATION.mdREADME.mdapp/app.goapp/app_test.gocli/commander/layout.gocli/example_test.gocli/export.gocli/export_test.gocli/iostreams.gocli/printer/printer.gocli/terminal/browser.gocli/terminal/term.gocli/version/release.goconfig/config.gomiddleware/cors/cors.gomiddleware/errorz/errorz.goserver/server.goserver/server_test.go
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/lint.yaml
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- cli/version/release.go
- app/app.go
- cli/commander/layout.go
- server/server_test.go
- cli/terminal/browser.go
- config/config.go
- server/server.go
- cli/iostreams.go
Comment on lines
+196
to
+217
| 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(addr), | ||
| app.WithOnStart(func(_ context.Context) error { | ||
| return fmt.Errorf("migration failed") | ||
| }), | ||
| app.WithOnStop(func(_ context.Context) error { | ||
| cleanupRan = true | ||
| return nil | ||
| }), | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| err = a.Start(context.Background()) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "migration failed") | ||
| // OnStop should NOT run — it runs only on graceful shutdown. | ||
| // cleanup() (telemetry flush) runs, but not onStop hooks. | ||
| assert.False(t, cleanupRan, "onStop hooks should not run on startup failure") | ||
| }) |
There was a problem hiding this comment.
Subtest name contradicts its assertion.
The name "onStart failure returns error and runs cleanup" implies onStop/cleanup runs, but the test actually asserts cleanupRan == false (correct per stopHooks(ctx, i) semantics in app/app.go:102-108). Rename to match intent, e.g. "onStart failure returns error and skips onStop hooks", to avoid confusion for future readers.
- t.Run("onStart failure returns error and runs cleanup", func(t *testing.T) {
+ t.Run("onStart failure returns error and skips onStop hooks", func(t *testing.T) {📝 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
| 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(addr), | |
| app.WithOnStart(func(_ context.Context) error { | |
| return fmt.Errorf("migration failed") | |
| }), | |
| app.WithOnStop(func(_ context.Context) error { | |
| cleanupRan = true | |
| return nil | |
| }), | |
| ) | |
| require.NoError(t, err) | |
| err = a.Start(context.Background()) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), "migration failed") | |
| // OnStop should NOT run — it runs only on graceful shutdown. | |
| // cleanup() (telemetry flush) runs, but not onStop hooks. | |
| assert.False(t, cleanupRan, "onStop hooks should not run on startup failure") | |
| }) | |
| t.Run("onStart failure returns error and skips onStop hooks", func(t *testing.T) { | |
| addr := freeAddr(t) | |
| var cleanupRan bool | |
| a, err := app.New( | |
| app.WithAddr(addr), | |
| app.WithOnStart(func(_ context.Context) error { | |
| return fmt.Errorf("migration failed") | |
| }), | |
| app.WithOnStop(func(_ context.Context) error { | |
| cleanupRan = true | |
| return nil | |
| }), | |
| ) | |
| require.NoError(t, err) | |
| err = a.Start(context.Background()) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), "migration failed") | |
| // OnStop should NOT run — it runs only on graceful shutdown. | |
| // cleanup() (telemetry flush) runs, but not onStop hooks. | |
| assert.False(t, cleanupRan, "onStop hooks should not run on startup failure") | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/app_test.go` around lines 196 - 217, Rename the t.Run subtest title to
accurately reflect its assertion: change the string passed to t.Run (currently
"onStart failure returns error and runs cleanup") to something like "onStart
failure returns error and skips onStop hooks" so the test name matches the
expectation asserted in the test body; locate the t.Run invocation wrapping the
test case (the anonymous func that sets cleanupRan, uses app.New and asserts
cleanupRan is false) and update only the descriptive string.
Comment on lines
+201
to
+225
| func (e *jsonExporter) extractData(v reflect.Value) any { | ||
| switch v.Kind() { | ||
| case reflect.Ptr, reflect.Interface: | ||
| if !v.IsNil() { | ||
| return e.extractData(v.Elem()) | ||
| } | ||
| return nil | ||
| case reflect.Slice: | ||
| a := make([]any, v.Len()) | ||
| for i := 0; i < v.Len(); i++ { | ||
| a[i] = e.extractData(v.Index(i)) | ||
| } | ||
| return a | ||
| case reflect.Struct: | ||
| if v.CanAddr() { | ||
| if ex, ok := v.Addr().Interface().(Exportable); ok { | ||
| return ex.ExportData(e.fields) | ||
| } | ||
| } | ||
| if ex, ok := v.Interface().(Exportable); ok { | ||
| return ex.ExportData(e.fields) | ||
| } | ||
| return StructExportData(v.Interface(), e.fields) | ||
| } | ||
| return v.Interface() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether extractData guards invalid reflect values and whether nil export coverage exists.
rg -n -C3 'func \(e \*jsonExporter\) extractData|IsValid\(|Interface\(\)|Write\(.*nil|nil data' cli/export.go cli/export_test.goRepository: raystack/salt
Length of output: 1786
🏁 Script executed:
#!/bin/bash
# Check the Write function implementation and its calls to extractData
rg -n -A10 'func \(e \*jsonExporter\) Write' cli/export.go | head -30
# Check test file for nil value coverage
rg -n 'nil|NULL|null|Write' cli/export_test.go | head -20Repository: raystack/salt
Length of output: 946
🏁 Script executed:
#!/bin/bash
# Check if there are any test cases passing nil to Write
rg -n 'Write.*nil|Write\(.*,\s*nil' cli/export_test.go
# Also check reflect.ValueOf behavior with nil
python3 << 'EOF'
import subprocess
# Verify Go's reflect.ValueOf(nil) behavior
code = '''package main
import (
"fmt"
"reflect"
)
func main() {
v := reflect.ValueOf(nil)
fmt.Printf("IsValid: %v\n", v.IsValid())
fmt.Printf("Kind: %v\n", v.Kind())
// Calling Interface() on invalid would panic, so we check first
if v.IsValid() {
fmt.Printf("Interface: %v\n", v.Interface())
}
}
'''
with open('/tmp/test.go', 'w') as f:
f.write(code)
result = subprocess.run(['go', 'run', '/tmp/test.go'], capture_output=True, text=True)
print("Output:", result.stdout)
if result.stderr:
print("Stderr:", result.stderr)
EOFRepository: raystack/salt
Length of output: 597
🏁 Script executed:
#!/bin/bash
# Simpler check: look for any test that passes nil data to Write
cat cli/export_test.go | grep -n "Write\|nil" | head -40Repository: raystack/salt
Length of output: 630
🏁 Script executed:
#!/bin/bash
# Read the test file to see what data is passed to Write
cat -n cli/export_test.go | sed -n '60,130p'Repository: raystack/salt
Length of output: 2740
Handle invalid reflection values before calling Interface().
Write(nil) reaches extractData(reflect.ValueOf(nil)); the invalid value misses every switch case and then panics at line 225 when calling Interface(). Return nil for invalid values so JSON encoding emits null. There is no test coverage for nil/invalid values.
Suggested fix
func (e *jsonExporter) extractData(v reflect.Value) any {
+ if !v.IsValid() {
+ return nil
+ }
switch v.Kind() {
case reflect.Ptr, reflect.Interface:
if !v.IsNil() {
return e.extractData(v.Elem())
}📝 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
| func (e *jsonExporter) extractData(v reflect.Value) any { | |
| switch v.Kind() { | |
| case reflect.Ptr, reflect.Interface: | |
| if !v.IsNil() { | |
| return e.extractData(v.Elem()) | |
| } | |
| return nil | |
| case reflect.Slice: | |
| a := make([]any, v.Len()) | |
| for i := 0; i < v.Len(); i++ { | |
| a[i] = e.extractData(v.Index(i)) | |
| } | |
| return a | |
| case reflect.Struct: | |
| if v.CanAddr() { | |
| if ex, ok := v.Addr().Interface().(Exportable); ok { | |
| return ex.ExportData(e.fields) | |
| } | |
| } | |
| if ex, ok := v.Interface().(Exportable); ok { | |
| return ex.ExportData(e.fields) | |
| } | |
| return StructExportData(v.Interface(), e.fields) | |
| } | |
| return v.Interface() | |
| func (e *jsonExporter) extractData(v reflect.Value) any { | |
| if !v.IsValid() { | |
| return nil | |
| } | |
| switch v.Kind() { | |
| case reflect.Ptr, reflect.Interface: | |
| if !v.IsNil() { | |
| return e.extractData(v.Elem()) | |
| } | |
| return nil | |
| case reflect.Slice: | |
| a := make([]any, v.Len()) | |
| for i := 0; i < v.Len(); i++ { | |
| a[i] = e.extractData(v.Index(i)) | |
| } | |
| return a | |
| case reflect.Struct: | |
| if v.CanAddr() { | |
| if ex, ok := v.Addr().Interface().(Exportable); ok { | |
| return ex.ExportData(e.fields) | |
| } | |
| } | |
| if ex, ok := v.Interface().(Exportable); ok { | |
| return ex.ExportData(e.fields) | |
| } | |
| return StructExportData(v.Interface(), e.fields) | |
| } | |
| return v.Interface() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/export.go` around lines 201 - 225, The extractData method in jsonExporter
must guard against invalid reflect.Value before using it: add an early check in
extractData (the function named extractData on type jsonExporter) to return nil
if v.IsValid() is false so you never call v.Interface() on an invalid value
(this fixes the Write(nil) -> extractData(reflect.ValueOf(nil)) panic); keep the
existing switch logic for valid kinds and ensure invalid or nil values return
nil so JSON encodes them as null.
Comment on lines
+14
to
+18
| func IsCI() bool { | ||
| return os.Getenv("CI") != "" || // GitHub Actions, Travis CI, CircleCI, Cirrus CI, GitLab CI, AppVeyor, CodeShip, dsari | ||
| os.Getenv("BUILD_NUMBER") != "" || // Jenkins, TeamCity | ||
| os.Getenv("RUN_ID") != "" // TaskCluster, dsari | ||
| } |
There was a problem hiding this comment.
Avoid treating CI=false as CI.
Line 15 classifies any non-empty CI value as CI, so CI=false or CI=0 would incorrectly disable interactive CLI behavior.
Proposed fix
import (
"fmt"
"os"
"os/exec"
+ "strings"
"github.com/mattn/go-isatty"
)
@@
func IsCI() bool {
- return os.Getenv("CI") != "" || // GitHub Actions, Travis CI, CircleCI, Cirrus CI, GitLab CI, AppVeyor, CodeShip, dsari
+ ci := strings.ToLower(strings.TrimSpace(os.Getenv("CI")))
+ return (ci != "" && ci != "false" && ci != "0") || // GitHub Actions, Travis CI, CircleCI, Cirrus CI, GitLab CI, AppVeyor, CodeShip, dsari
os.Getenv("BUILD_NUMBER") != "" || // Jenkins, TeamCity
os.Getenv("RUN_ID") != "" // TaskCluster, dsari
}📝 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
| func IsCI() bool { | |
| return os.Getenv("CI") != "" || // GitHub Actions, Travis CI, CircleCI, Cirrus CI, GitLab CI, AppVeyor, CodeShip, dsari | |
| os.Getenv("BUILD_NUMBER") != "" || // Jenkins, TeamCity | |
| os.Getenv("RUN_ID") != "" // TaskCluster, dsari | |
| } | |
| func IsCI() bool { | |
| ci := strings.ToLower(strings.TrimSpace(os.Getenv("CI"))) | |
| return (ci != "" && ci != "false" && ci != "0") || // GitHub Actions, Travis CI, CircleCI, Cirrus CI, GitLab CI, AppVeyor, CodeShip, dsari | |
| os.Getenv("BUILD_NUMBER") != "" || // Jenkins, TeamCity | |
| os.Getenv("RUN_ID") != "" // TaskCluster, dsari | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/terminal/term.go` around lines 14 - 18, The IsCI function treats any
non-empty CI env value as true, so values like "false" or "0" incorrectly
indicate CI; update IsCI to read os.Getenv("CI"), normalize (strings.TrimSpace +
strings.ToLower) and treat it as CI only if non-empty and not equal to "false"
or "0" (keep existing checks for BUILD_NUMBER and RUN_ID unchanged); reference
the IsCI function and the "CI", "BUILD_NUMBER", and "RUN_ID" environment
variable checks when making this change.
Comment on lines
+25
to
+29
| func OpenBrowser(goos, url string) (*exec.Cmd, error) { | ||
| if !isatty.IsTerminal(os.Stdout.Fd()) && !isatty.IsCygwinTerminal(os.Stdout.Fd()) { | ||
| return nil, fmt.Errorf("OpenBrowser requires a TTY on stdout") | ||
| } | ||
| return openBrowserCmd(goos, url), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the OpenBrowser function
cat -n cli/terminal/term.goRepository: raystack/salt
Length of output: 1310
🏁 Script executed:
# Check for IOStreams definition and usage patterns
fd -t f IOStreams . | head -20Repository: raystack/salt
Length of output: 39
🏁 Script executed:
# Search for IOStreams usage in the codebase
rg -t go "IOStreams" --max-count 10Repository: raystack/salt
Length of output: 2211
🏁 Script executed:
# Check how stdin/stderr TTY detection is used elsewhere
rg -t go "IsTerminal.*Stdin|IsCygwinTerminal.*Stdin"Repository: raystack/salt
Length of output: 39
🏁 Script executed:
# Look for callers of OpenBrowser
rg -t go "OpenBrowser"Repository: raystack/salt
Length of output: 307
🏁 Script executed:
# Check for --json flag handling and stream context
rg -t go "\-\-json" --max-count 10Repository: raystack/salt
Length of output: 1229
🏁 Script executed:
# Search for calls to OpenBrowser (not just definition)
rg -t go "OpenBrowser\(" -B 3 -A 3Repository: raystack/salt
Length of output: 535
🏁 Script executed:
# Search for any type that has OpenBrowser as a method
rg -t go "func.*OpenBrowser"Repository: raystack/salt
Length of output: 133
🏁 Script executed:
# Check the openBrowserCmd function to understand the flow
rg -t go "func openBrowserCmd"Repository: raystack/salt
Length of output: 130
🏁 Script executed:
# Look for commands that might use browser opening
rg -t go "browser|Browser" -i -t go --max-count 20Repository: raystack/salt
Length of output: 634
🏁 Script executed:
# Search more broadly - maybe it's called without the full function name or in imports
rg -t go "OpenBrowser" --type-listRepository: raystack/salt
Length of output: 446
🏁 Script executed:
# Check the terminal package for all public functions
rg -t go "^func [A-Z]" cli/terminal/term.goRepository: raystack/salt
Length of output: 131
🏁 Script executed:
# Look at the full context of the terminal package
ls -la cli/terminal/Repository: raystack/salt
Length of output: 400
🏁 Script executed:
# Check IOStreams implementation to understand the stream checking pattern
head -100 cli/iostreams.goRepository: raystack/salt
Length of output: 2919
🏁 Script executed:
# Search for any use of stdin/stdout TTY checks in the codebase
rg -t go "IsStdinTTY|IsStdoutTTY|Stdin.Fd|Stdout.Fd" -B 2 -A 2Repository: raystack/salt
Length of output: 1843
🏁 Script executed:
# Look at browser.go to understand the full context
cat -n cli/terminal/browser.goRepository: raystack/salt
Length of output: 1155
🏁 Script executed:
# Check if there are any tests for OpenBrowser
rg -t go "TestOpenBrowser|openBrowser" -iRepository: raystack/salt
Length of output: 531
🏁 Script executed:
# Look for any use of terminal.OpenBrowser in the entire repo
rg -t go "terminal\.OpenBrowser"Repository: raystack/salt
Length of output: 39
🏁 Script executed:
# Check the context around export.go to understand the pattern of IOStreams usage
cat -n cli/export.go | head -80Repository: raystack/salt
Length of output: 3217
Check stdin TTY instead of stdout to unblock browser launch with piped output.
Line 26 incorrectly gates browser opening on stdout being a TTY. This blocks legitimate scenarios where stdout is redirected for structured output (e.g., --json output piped to jq) but the user is still interacting at the terminal via stdin. A user at an interactive terminal with redirected stdout can still open a browser—what matters is whether stdin is a TTY, not stdout. Refactor to accept the interactive stream as a parameter (e.g., stdin fd or IOStreams from context) rather than hard-coding os.Stdout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/terminal/term.go` around lines 25 - 29, The OpenBrowser function
currently checks stdout TTY which blocks browser launches when stdout is
redirected; change OpenBrowser(goos, url string) to accept an interactive stream
(e.g., add a parameter like in io.Reader or an OS file descriptor/streams
struct) and use that stream's file descriptor with isatty.IsTerminal /
IsCygwinTerminal instead of os.Stdout.Fd(); update the internal check in
OpenBrowser and keep returning openBrowserCmd(goos, url), and update all callers
to pass os.Stdin (or the IOStreams.Context stdin) so interactivity is determined
by stdin rather than stdout.
Comment on lines
+40
to
+57
| // Defaults returns sensible CORS defaults for ConnectRPC services. | ||
| // Includes Connect-specific headers. | ||
| func Defaults() []Option { | ||
| return []Option{ | ||
| WithAllowedOrigins("*"), | ||
| WithAllowedMethods("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS"), | ||
| WithAllowedHeaders( | ||
| "Content-Type", | ||
| "Connect-Protocol-Version", | ||
| "Connect-Timeout-Ms", | ||
| "Grpc-Timeout", | ||
| "X-Grpc-Web", | ||
| "X-User-Agent", | ||
| "X-Request-ID", | ||
| "Authorization", | ||
| ), | ||
| WithMaxAge(7200), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
connectrpc.com/cors AllowedMethods ExposedHeaders Access-Control-Expose-Headers Connect browser clients
💡 Result:
For ConnectRPC browser clients to work correctly with CORS (connectrpc.com/cors), servers must configure specific CORS headers. Both Connect and gRPC-Web protocols require CORS preflights for cross-origin requests. Connect protocol: - Access-Control-Allow-Methods: GET, POST - Access-Control-Allow-Headers: Content-Type, Connect-Protocol-Version, Connect-Timeout-Ms, X-User-Agent - Access-Control-Expose-Headers: (not explicitly required for Connect in basic config, but for full gRPC-Web compatibility: Grpc-Status, Grpc-Message, Grpc-Status-Details-Bin) gRPC-Web protocol: - Access-Control-Allow-Methods: POST - Access-Control-Allow-Headers: Content-Type, Grpc-Timeout, X-Grpc-Web, X-User-Agent - Access-Control-Expose-Headers: Grpc-Status, Grpc-Message, Grpc-Status-Details-Bin To support both protocols (recommended), use the combined preflight response: Access-Control-Allow-Methods: GET, POST Access-Control-Allow-Headers: Content-Type, Connect-Protocol-Version, Connect-Timeout-Ms, Grpc-Timeout, X-Grpc-Web, X-User-Agent Access-Control-Max-Age: 7200 (2 hours) Access-Control-Allow-Origin: specific origin (e.g., https://connectrpc.com), avoid * with credentials Vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers The official Go package connectrpc.com/cors provides helpers: - AllowedMethods: returns required methods - AllowedHeaders: returns required headers - ExposedHeaders: returns required exposed headers (Grpc-Status, etc.) Example preflight response (HTTP/1.1 204 No Content): Access-Control-Allow-Origin: https://connectrpc.com Access-Control-Allow-Methods: POST, GET Access-Control-Allow-Headers: Content-Type,Connect-Protocol-Version,Connect-Timeout-Ms,Grpc-Timeout,X-Grpc-Web,X-User-Agent Access-Control-Max-Age: 7200 Vary: Origin,Access-Control-Request-Method,Access-Control-Request-Headers Unary GET requests for idempotent RPCs can avoid preflights if no custom headers are used and origin is allowed. Browser clients use @connectrpc/connect-web with createConnectTransport or createGrpcWebTransport, relying on fetch which triggers these CORS checks.
Citations:
- 1: https://connectrpc.com/docs/cors/
- 2: https://pkg.go.dev/connectrpc.com/cors
- 3: https://github.com/connectrpc/cors-go
Align Connect defaults with standard CORS requirements for gRPC-Web clients.
The Defaults() function omits Access-Control-Expose-Headers, preventing browser clients from reading gRPC response status headers (Grpc-Status, Grpc-Message, Grpc-Status-Details-Bin). Additionally, the default methods include PUT, DELETE, and PATCH, which exceed Connect's standard GET/POST set—restrict these to opt-in via WithAllowedMethods().
The official connectrpc.com/cors package documents both ExposedHeaders() for response headers and AllowedMethods() for methods: https://pkg.go.dev/connectrpc.com/cors.
Proposed changes
type config struct {
allowedOrigins []string
allowedMethods []string
allowedHeaders []string
+ exposedHeaders []string
maxAge int
} func WithAllowedHeaders(headers ...string) Option {
return func(c *config) { c.allowedHeaders = headers }
}
+// WithExposedHeaders sets the response headers exposed to browser clients.
+func WithExposedHeaders(headers ...string) Option {
+ return func(c *config) { c.exposedHeaders = headers }
+}
+
// WithMaxAge sets the max age (in seconds) for preflight cache.
func WithMaxAge(seconds int) Option {
return func(c *config) { c.maxAge = seconds }
} func Defaults() []Option {
return []Option{
WithAllowedOrigins("*"),
- WithAllowedMethods("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS"),
+ WithAllowedMethods("GET", "POST"),
WithAllowedHeaders(
"Content-Type",
"Connect-Protocol-Version",
"Connect-Timeout-Ms",
"Grpc-Timeout",
"X-Grpc-Web",
"X-User-Agent",
"X-Request-ID",
"Authorization",
),
+ WithExposedHeaders(
+ "Grpc-Status",
+ "Grpc-Message",
+ "Grpc-Status-Details-Bin",
+ ),
WithMaxAge(7200),
}
} w.Header().Set("Access-Control-Allow-Origin", origin)
w.Header().Set("Access-Control-Allow-Methods", strings.Join(cfg.allowedMethods, ", "))
w.Header().Set("Access-Control-Allow-Headers", strings.Join(cfg.allowedHeaders, ", "))
+ if len(cfg.exposedHeaders) > 0 {
+ w.Header().Set("Access-Control-Expose-Headers", strings.Join(cfg.exposedHeaders, ", "))
+ }
if cfg.maxAge > 0 {📝 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
| // Defaults returns sensible CORS defaults for ConnectRPC services. | |
| // Includes Connect-specific headers. | |
| func Defaults() []Option { | |
| return []Option{ | |
| WithAllowedOrigins("*"), | |
| WithAllowedMethods("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS"), | |
| WithAllowedHeaders( | |
| "Content-Type", | |
| "Connect-Protocol-Version", | |
| "Connect-Timeout-Ms", | |
| "Grpc-Timeout", | |
| "X-Grpc-Web", | |
| "X-User-Agent", | |
| "X-Request-ID", | |
| "Authorization", | |
| ), | |
| WithMaxAge(7200), | |
| } | |
| // Defaults returns sensible CORS defaults for ConnectRPC services. | |
| // Includes Connect-specific headers. | |
| func Defaults() []Option { | |
| return []Option{ | |
| WithAllowedOrigins("*"), | |
| WithAllowedMethods("GET", "POST"), | |
| WithAllowedHeaders( | |
| "Content-Type", | |
| "Connect-Protocol-Version", | |
| "Connect-Timeout-Ms", | |
| "Grpc-Timeout", | |
| "X-Grpc-Web", | |
| "X-User-Agent", | |
| "X-Request-ID", | |
| "Authorization", | |
| ), | |
| WithExposedHeaders( | |
| "Grpc-Status", | |
| "Grpc-Message", | |
| "Grpc-Status-Details-Bin", | |
| ), | |
| WithMaxAge(7200), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/cors/cors.go` around lines 40 - 57, Update Defaults() in cors.go
to align with ConnectRPC CORS expectations: restrict the default allowed methods
to only "GET", "POST", "OPTIONS" by changing the WithAllowedMethods(...) call so
extra methods ("PUT","DELETE","PATCH") are not included (developers can opt into
them via WithAllowedMethods), and add a WithExposedHeaders(...) entry exposing
"Grpc-Status", "Grpc-Message", and "Grpc-Status-Details-Bin" so browser gRPC-web
clients can read response status headers; keep existing WithAllowedHeaders and
WithMaxAge as-is.
| return | ||
| } | ||
|
|
||
| w.Header().Set("Vary", "Origin") |
There was a problem hiding this comment.
Preserve existing Vary values.
Set("Vary", "Origin") can overwrite values already added by surrounding middleware or handlers. Append Origin only if it is not already present.
🛠️ Proposed fix
- w.Header().Set("Vary", "Origin")
+ addVary(w.Header(), "Origin") func isOriginAllowed(allowed []string, origin string) bool {
for _, a := range allowed {
if a == "*" || a == origin {
return true
}
}
return false
}
+
+func addVary(h http.Header, value string) {
+ for _, existing := range h.Values("Vary") {
+ for _, part := range strings.Split(existing, ",") {
+ if strings.EqualFold(strings.TrimSpace(part), value) {
+ return
+ }
+ }
+ }
+ h.Add("Vary", value)
+}📝 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
| w.Header().Set("Vary", "Origin") | |
| addVary(w.Header(), "Origin") |
Suggested change
| w.Header().Set("Vary", "Origin") | |
| func isOriginAllowed(allowed []string, origin string) bool { | |
| for _, a := range allowed { | |
| if a == "*" || a == origin { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| func addVary(h http.Header, value string) { | |
| for _, existing := range h.Values("Vary") { | |
| for _, part := range strings.Split(existing, ",") { | |
| if strings.EqualFold(strings.TrimSpace(part), value) { | |
| return | |
| } | |
| } | |
| } | |
| h.Add("Vary", value) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/cors/cors.go` at line 83, The Vary header is being overwritten by
w.Header().Set("Vary", "Origin"); instead read the existing value via
w.Header().Get("Vary"), check case-insensitively whether "Origin" is already
present (split on commas, trim spaces), and only append ", Origin" when missing,
then write back with w.Header().Set("Vary", newValue); locate this change where
w.Header().Set("Vary", "Origin") is called in the cors middleware and replace it
with the conditional-get/append logic so existing Vary entries are preserved.
Comment on lines
+85
to
+104
| if !isOriginAllowed(cfg.allowedOrigins, origin) { | ||
| if r.Method == http.MethodOptions { | ||
| w.WriteHeader(http.StatusForbidden) | ||
| return | ||
| } | ||
| next.ServeHTTP(w, r) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Access-Control-Allow-Origin", origin) | ||
| w.Header().Set("Access-Control-Allow-Methods", strings.Join(cfg.allowedMethods, ", ")) | ||
| w.Header().Set("Access-Control-Allow-Headers", strings.Join(cfg.allowedHeaders, ", ")) | ||
|
|
||
| if cfg.maxAge > 0 { | ||
| w.Header().Set("Access-Control-Max-Age", strconv.Itoa(cfg.maxAge)) | ||
| } | ||
|
|
||
| if r.Method == http.MethodOptions { | ||
| w.WriteHeader(http.StatusNoContent) | ||
| return |
There was a problem hiding this comment.
Only short-circuit real preflight requests.
This currently intercepts any OPTIONS request with Origin, including application-level OPTIONS handlers. Gate the 204/403 behavior on Access-Control-Request-Method.
🐛 Proposed fix
- if !isOriginAllowed(cfg.allowedOrigins, origin) {
- if r.Method == http.MethodOptions {
+ isPreflight := r.Method == http.MethodOptions && r.Header.Get("Access-Control-Request-Method") != ""
+
+ if !isOriginAllowed(cfg.allowedOrigins, origin) {
+ if isPreflight {
w.WriteHeader(http.StatusForbidden)
return
}
next.ServeHTTP(w, r)
return
@@
- if r.Method == http.MethodOptions {
+ if isPreflight {
w.WriteHeader(http.StatusNoContent)
return
}📝 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
| if !isOriginAllowed(cfg.allowedOrigins, origin) { | |
| if r.Method == http.MethodOptions { | |
| w.WriteHeader(http.StatusForbidden) | |
| return | |
| } | |
| next.ServeHTTP(w, r) | |
| return | |
| } | |
| w.Header().Set("Access-Control-Allow-Origin", origin) | |
| w.Header().Set("Access-Control-Allow-Methods", strings.Join(cfg.allowedMethods, ", ")) | |
| w.Header().Set("Access-Control-Allow-Headers", strings.Join(cfg.allowedHeaders, ", ")) | |
| if cfg.maxAge > 0 { | |
| w.Header().Set("Access-Control-Max-Age", strconv.Itoa(cfg.maxAge)) | |
| } | |
| if r.Method == http.MethodOptions { | |
| w.WriteHeader(http.StatusNoContent) | |
| return | |
| isPreflight := r.Method == http.MethodOptions && r.Header.Get("Access-Control-Request-Method") != "" | |
| if !isOriginAllowed(cfg.allowedOrigins, origin) { | |
| if isPreflight { | |
| w.WriteHeader(http.StatusForbidden) | |
| return | |
| } | |
| next.ServeHTTP(w, r) | |
| return | |
| } | |
| w.Header().Set("Access-Control-Allow-Origin", origin) | |
| w.Header().Set("Access-Control-Allow-Methods", strings.Join(cfg.allowedMethods, ", ")) | |
| w.Header().Set("Access-Control-Allow-Headers", strings.Join(cfg.allowedHeaders, ", ")) | |
| if cfg.maxAge > 0 { | |
| w.Header().Set("Access-Control-Max-Age", strconv.Itoa(cfg.maxAge)) | |
| } | |
| if isPreflight { | |
| w.WriteHeader(http.StatusNoContent) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/cors/cors.go` around lines 85 - 104, The current middleware
short-circuits every OPTIONS request with an Origin regardless of whether it's a
CORS preflight; update the logic to only treat OPTIONS as preflight when the
Access-Control-Request-Method header is present: in the branch that checks
!isOriginAllowed(cfg.allowedOrigins, origin) replace the unconditional r.Method
== http.MethodOptions short-circuit with a check for
r.Header.Get("Access-Control-Request-Method") != "" and only then write
StatusForbidden, otherwise call next.ServeHTTP; likewise in the allowed-origin
branch ensure the r.Method == http.MethodOptions -> StatusNoContent behavior is
gated on the same Access-Control-Request-Method header so application-level
OPTIONS handlers run when that header is absent.
Comment on lines
+30
to
+39
| func WithLogger(l *slog.Logger) Option { | ||
| return func(c *config) { c.logger = l } | ||
| } | ||
|
|
||
| func newConfig(opts []Option) *config { | ||
| c := &config{logger: slog.New(slog.DiscardHandler)} | ||
| for _, opt := range opts { | ||
| opt(c) | ||
| } | ||
| return c |
There was a problem hiding this comment.
Keep the discard logger fallback when WithLogger(nil) is passed.
Line 31 can overwrite the safe default with nil, so the later cfg.logger.Error(...) calls will panic while handling an error. Add a post-options fallback or ignore nil loggers.
🛡️ Proposed fix
func newConfig(opts []Option) *config {
c := &config{logger: slog.New(slog.DiscardHandler)}
for _, opt := range opts {
opt(c)
}
+ if c.logger == nil {
+ c.logger = slog.New(slog.DiscardHandler)
+ }
return c
}📝 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
| func WithLogger(l *slog.Logger) Option { | |
| return func(c *config) { c.logger = l } | |
| } | |
| func newConfig(opts []Option) *config { | |
| c := &config{logger: slog.New(slog.DiscardHandler)} | |
| for _, opt := range opts { | |
| opt(c) | |
| } | |
| return c | |
| func WithLogger(l *slog.Logger) Option { | |
| return func(c *config) { c.logger = l } | |
| } | |
| func newConfig(opts []Option) *config { | |
| c := &config{logger: slog.New(slog.DiscardHandler)} | |
| for _, opt := range opts { | |
| opt(c) | |
| } | |
| if c.logger == nil { | |
| c.logger = slog.New(slog.DiscardHandler) | |
| } | |
| return c | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/errorz/errorz.go` around lines 30 - 39, The WithLogger option
currently allows setting c.logger to nil which can cause panics; keep the safe
discard logger fallback by ensuring newConfig enforces a non-nil logger after
applying opts: in newConfig (and/or in WithLogger) check if c.logger is nil
after the opts loop and, if so, reset it to slog.New(slog.DiscardHandler()) so
that config.logger never ends up nil when code later calls
cfg.logger.Error(...).
- Preserve test-injected IOStreams in Init's PersistentPreRunE instead of always overwriting with System() - Use cmd.OutOrStdout() in completion command instead of os.Stdout - Disable HTML escaping in Output.JSON/PrettyJSON to match Exporter.Write - Add integration tests (9 sample CLIs, 34 tests) - Add edge case tests (20 groups, 80+ subtests) covering nested commands, JSON export edge cases, table output, error propagation, hooks, and more
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
1 participant
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
Evolves salt from a bag of independent utilities into the standard way to build raystack services and CLIs. Drops unused packages, adds bootstrap entry points, modernizes dependencies.
18 packages, 52 files, 7060 lines. Down from 25+ packages.
Service bootstrap (
app/)H2C and health check at
/pingby default. No hidden middleware — everything is explicit.CLI bootstrap (
cli/)Initadds help, completion, reference docs, and silences Cobra's default error/usage output.Executeruns the command and handles all errors with proper exit codes — sentinel errors (ErrSilent,ErrCancel), flag errors (with contextual usage), and general errors. Commands access shared output viacli.Output(cmd)and prompting viacli.Prompter(cmd).Error handling
cli.Executeowns all error dispatch:cli.ErrSilentcli.ErrCancelFlagError,HandleError,NewFlagError, andcommander.IsCommandErrare removed — their functionality is internal toInit/Execute.New packages
app/— service lifecycle with graceful shutdowncli/— CLI lifecycle withInit+Executeserver/— h2c HTTP server with health checks, timeouts, middleware supportmiddleware/— recovery, requestid, requestlog, errorz, cors (Connect + HTTP)Packages dropped (zero consumers)
db/— projects use their own DB libraryauth/oidc/— incomplete, tracked in feat: CLI authentication — login, token storage, auto-refresh #86auth/audit/— too simple, tracked in feat: standardized audit logging schema and service #87testkit/— projects use dockertest directlyobservability/logger/— use*slog.Loggerfrom stdlibobservability/otelgrpc/— use otelconnectobservability/otelhttpclient/— use otelhttpserver/mux/— replaced by new h2c serverPackages restructured
observability/telemetry/cli/terminatorcli/terminalcli/promptercli/promptcli/releasercli/versionPackages improved
config/— upgraded validator v9→v10, replaced unmaintained go-defaults, removed stdout printingcli/printer/— rewritten as Output type (testable, writer-based)cli/prompt/— migrated from archived survey/v2 to charmbracelet/huhserver/— read/write/idle timeouts, WithServer passthroughCode modernization
interface{}→anyacross all packagesos.IsNotExist→errors.Is(err, fs.ErrNotExist)sort.Slice→slices.SortFuncSilenceErrors/SilenceUsageset inInit(follows gh CLI pattern)Dependencies
Removed: zap, logrus, survey/v2, tablewriter, oklog/run, safeexec, pkg/errors, go-defaults, sqlx, golang-migrate, dockertest
Added: connectrpc.com/connect, charmbracelet/huh, creasty/defaults
Upgraded: cobra v1.8→v1.10, validator v9→v10, glamour v0.3→v1.0, termenv v0.11→v0.16, spinner v1.18→v1.23, progressbar v3.8→v3.19
CI
golangci-lint v1.60→v2.11, Go 1.22→1.24, dropped errcheck linter.
Docs
Test plan
go build ./...passesgo vet ./...cleangolangci-lint run ./...— 0 issuesgo test ./...— all passapp.Run()andcli.Execute()