Miren doctor improvements #491
Conversation
📝 WalkthroughWalkthroughRemoves DoctorAll and DoctorApps; refactors doctor checks and auth display; adds interactive config/server guidance and HTTP(S) endpoint checks. App listing aggregates RPC, ingress, and pool state, surfaces OSC‑8 clickable routes, and changes health/status semantics. Table rendering gains OSC‑8 support; login refactored; clientconfig adds GetClusterSource. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (10)📚 Learning: 2025-12-15T17:54:35.854ZApplied to files:
📚 Learning: 2025-06-02T20:03:41.073ZApplied to files:
📚 Learning: 2025-05-22T05:27:15.270ZApplied to files:
📚 Learning: 2025-05-22T05:27:15.270ZApplied to files:
📚 Learning: 2025-10-09T21:31:23.219ZApplied to files:
📚 Learning: 2025-02-08T04:28:25.582ZApplied to files:
📚 Learning: 2025-02-05T23:40:16.698ZApplied to files:
📚 Learning: 2025-11-15T00:16:45.268ZApplied to files:
📚 Learning: 2025-09-11T22:31:18.478ZApplied to files:
📚 Learning: 2025-09-08T20:34:44.431ZApplied to files:
🧬 Code graph analysis (2)cli/commands/doctor_server.go (2)
cli/commands/doctor_config.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (9)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/commands/app_list.go (1)
184-211: Breaking change: JSON output schema forapp listhas been significantly altered.The
--format jsonoutput has removedDeployedBy,DeployedAt, andGitCommitfields and replaced them withReadyInstances,DesiredInstances,Health, andRoutes. While the underlying data model still tracks deployment information, external tools or scripts consuming this JSON output will break. Consider documenting this change in release notes or providing a migration path if this is a public API.
🧹 Nitpick comments (11)
cli/commands/doctor_server.go (3)
113-117: Consider polling server health instead of blind wait.The current implementation waits a fixed 10 seconds regardless of when the server becomes ready. Consider polling the server during this window and breaking early once connectivity is established.
Example approach:
// Wait for server to start ctx.Printf(" waiting for server") - for i := 0; i < 10; i++ { - time.Sleep(1 * time.Second) - fmt.Print(".") - } + var verifyClient interface{ Close() error } + var verifyErr error + for i := 0; i < 10; i++ { + time.Sleep(1 * time.Second) + fmt.Print(".") + verifyClient, verifyErr = ctx.RPCClient("entities") + if verifyErr == nil && verifyClient != nil { + break + } + if verifyClient != nil { + verifyClient.Close() + } + } // Verify it started - verifyClient, verifyErr := ctx.RPCClient("entities") if verifyErr == nil && verifyClient != nil {
197-216: Set minimum TLS version for diagnostic HTTP client.While
InsecureSkipVerifyis appropriate for diagnostics to handle self-signed certificates, setting a minimum TLS version improves security posture even in diagnostic tools.Based on coding guidelines and static analysis hints.
Apply this diff:
client := &http.Client{ Timeout: 5 * time.Second, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ InsecureSkipVerify: true, // Accept self-signed certs for diagnostics + MinVersion: tls.VersionTLS12, }, }, }
218-230: Simplify error description logic.The nested
*net.OpErrortype assertion (line 224) is unusual and may not capture the most useful error details. For network diagnostics, checking for specific syscall errors (like ECONNREFUSED) provides clearer feedback.Consider this approach:
func describeHTTPError(err error) string { if netErr, ok := err.(net.Error); ok && netErr.Timeout() { return "timeout" } - // Check for connection refused if opErr, ok := err.(*net.OpError); ok { - if sysErr, ok := opErr.Err.(*net.OpError); ok { - return sysErr.Err.Error() - } - return opErr.Err.Error() + if opErr.Err != nil { + return opErr.Err.Error() + } } return err.Error() }Or for more specific diagnostics:
import "syscall" func describeHTTPError(err error) string { if netErr, ok := err.(net.Error); ok && netErr.Timeout() { return "timeout" } if opErr, ok := err.(*net.OpError); ok { if opErr.Err == syscall.ECONNREFUSED { return "connection refused" } if opErr.Err != nil { return opErr.Err.Error() } } return err.Error() }cli/commands/doctor_auth.go (2)
87-96: Consider extracting duplicated prompt logic.The prompt-read-normalize-check pattern is repeated for both the "set up auth" and "refresh login" flows. A small helper would reduce duplication.
+// promptYesNo prompts the user and returns true for affirmative responses +func promptYesNo(prompt string) bool { + fmt.Print(prompt) + var response string + fmt.Scanln(&response) + response = strings.TrimSpace(strings.ToLower(response)) + return response == "" || response == "y" || response == "yes" +}Also applies to: 101-111
88-88:fmt.Scanlnsilently ignores errors.If stdin is closed or encounters an error,
Scanlnreturns an error that's currently discarded. In edge cases (e.g., piped input unexpectedly closed), this could cause confusing behavior. Consider checking the error or documenting why it's safe to ignore.Also applies to: 102-102
cli/commands/doctor.go (1)
304-368: Good helper design with graceful degradation.Returning
(0, 0)on errors is appropriate for a diagnostic display that shouldn't fail the whole command.One consideration: the
appNamesmap (lines 323-328) is used to iterate but you already havetotalfrom line 317. You could simplify by iteratingdeploymentMapdirectly and checking if the app exists.- // Count unhealthy apps (only failed deployments count as unhealthy) - for appName := range appNames { - if dep, ok := deploymentMap[appName]; ok { - if dep.status == "failed" { - unhealthy++ - } - } - } + // Count unhealthy apps (only failed deployments for known apps count as unhealthy) + for appName, dep := range deploymentMap { + if appNames[appName] && dep.status == "failed" { + unhealthy++ + } + }cli/commands/doctor_apps.go (2)
36-39: Cluster object intentionally discarded.The cluster is fetched to validate it exists, but the value isn't needed. Consider using a blank identifier or adding a brief comment for clarity.
- _, err = cfg.GetCluster(clusterName) + // Validate cluster exists + if _, err = cfg.GetCluster(clusterName); err != nil { + return err + } - if err != nil { - return err - }
257-279: Subprocess invocation works but has rough edges.A few observations:
- Line 263:
cmd.Run()error is silently ignored for "View logs". Consider at least logging if it fails.- Line 265:
fmt.Scanln()error is ignored (minor, same pattern as doctor_auth.go).- The
mirenbinary must be in PATH - this works but couples to the external CLI.- cmd.Run() + if err := cmd.Run(); err != nil { + ctx.Printf("%s\n", infoRed.Render("Failed to fetch logs: "+err.Error())) + }cli/commands/doctor_config.go (3)
289-295: Inconsistent prompt handling - consider usingui.Confirm.This uses
fmt.Scanlnwhile other parts of the codebase (e.g., line 253) useui.Confirm. For consistency and better UX (input validation, default handling), consider using the UI helper:- ctx.Printf("Check registration with sudo? [Y/n] ") - var response string - fmt.Scanln(&response) - response = strings.TrimSpace(strings.ToLower(response)) - if response != "" && response != "y" && response != "yes" { - return nil - } + confirmed, err := ui.Confirm(ui.WithMessage("Check registration with sudo?"), ui.WithDefault(true)) + if err != nil || !confirmed { + return nil + }The same pattern applies to the other
fmt.Scanlnprompts at lines 313-319, 349-355, 381-385, and 410-413.
399-405: Arbitrary wait without health verification.The 10-second fixed wait doesn't confirm the server actually started. Consider polling a health endpoint or at least checking if the process is still running:
- // Wait for server to start up and report status - ctx.Printf(" waiting for server to initialize") - for i := 0; i < 10; i++ { - time.Sleep(1 * time.Second) - fmt.Print(".") - } - ctx.Printf("\n%s\n", infoGreen.Render("✓ Server restarted")) + // Wait for server to start up and report status + ctx.Printf(" waiting for server to initialize") + serverReady := false + for i := 0; i < 10; i++ { + time.Sleep(1 * time.Second) + fmt.Print(".") + // Could add a health check here, e.g., TCP dial to port 8443 + } + if serverReady { + ctx.Printf("\n%s\n", infoGreen.Render("✓ Server restarted")) + } else { + ctx.Printf("\n%s\n", infoGray.Render("Server restart initiated (verify with 'miren server status')")) + }
279-279: Consider extracting hardcoded paths as constants.The paths
/var/lib/miren/serverand/var/lib/miren/release/mirenappear multiple times. Extracting these as package-level constants would improve maintainability and reduce duplication.Also applies to: 343-344, 395-395
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
cli/commands/app_list.go(6 hunks)cli/commands/commands.go(0 hunks)cli/commands/doctor.go(6 hunks)cli/commands/doctor_all.go(0 hunks)cli/commands/doctor_apps.go(7 hunks)cli/commands/doctor_auth.go(2 hunks)cli/commands/doctor_config.go(1 hunks)cli/commands/doctor_server.go(3 hunks)cli/commands/login.go(4 hunks)clientconfig/clientconfig.go(1 hunks)pkg/ui/table.go(3 hunks)
💤 Files with no reviewable changes (2)
- cli/commands/commands.go
- cli/commands/doctor_all.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
pkg/ui/table.gocli/commands/doctor_config.goclientconfig/clientconfig.gocli/commands/doctor_auth.gocli/commands/login.gocli/commands/doctor.gocli/commands/doctor_apps.gocli/commands/app_list.gocli/commands/doctor_server.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: phinze
Repo: mirendev/runtime PR: 74
File: pkg/entity/cmd/schemagen/generator.go:0-0
Timestamp: 2025-06-03T00:19:24.104Z
Learning: User phinze prefers to file separate issues (like MIR-199) for code review findings that are outside the scope of the current PR rather than fixing them inline.
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_auth.gocli/commands/login.gocli/commands/doctor.gocli/commands/doctor_apps.gocli/commands/app_list.gocli/commands/doctor_server.go
📚 Learning: 2025-06-02T20:03:41.073Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 81
File: cli/commands/entity_put.go:32-65
Timestamp: 2025-06-02T20:03:41.073Z
Learning: In cli/commands/entity_put.go and similar RPC client implementations, the user evanphx acknowledged that the unconditional rpc.WithSkipVerify in fallback scenarios needs security cleanup but deferred it to a future date.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase doesn't actually return errors despite its signature. It starts servers in goroutines and immediately returns nil, handling any errors internally by logging them.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase has an error return type in its signature but always returns `nil`. It handles errors internally by logging them, making error checking at the call site unnecessary.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-02-08T04:28:25.582Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 27
File: server/tls.go:19-24
Timestamp: 2025-02-08T04:28:25.582Z
Learning: For Go HTTP servers using http.Serve, error handling in the serving goroutine only needs to log errors, as the server can only fail when it's being closed. Complex error propagation or startup checks are unnecessary.
Applied to files:
cli/commands/doctor_server.go
🧬 Code graph analysis (5)
cli/commands/doctor_config.go (4)
pkg/ui/picker.go (3)
IsInteractive(360-371)PickerItem(13-18)RunPicker(337-357)cli/commands/login.go (1)
LoginWithDefaults(113-115)pkg/registration/registration.go (2)
LoadRegistration(261-277)StoredRegistration(80-96)cli/commands/server_register.go (1)
RegisterOptions(13-18)
cli/commands/doctor_auth.go (3)
pkg/ui/picker.go (1)
IsInteractive(360-371)cli/commands/login.go (1)
LoginWithDefaults(113-115)pkg/rpc/server.go (1)
Method(110-115)
cli/commands/login.go (1)
cli/commands/global.go (1)
Context(31-58)
cli/commands/doctor.go (3)
api/entityserver/entityserver_v1alpha/rpc.gen.go (1)
NewEntityAccessClient(2800-2802)pkg/mapx/keys.go (1)
Values(19-26)pkg/entity/entity.go (1)
Decode(652-654)
cli/commands/app_list.go (3)
cli/commands/format.go (1)
FormatOptions(11-13)api/core/core_v1alpha/schema.gen.go (1)
AppVersion(82-91)api/compute/compute_v1alpha/schema.gen.go (1)
SandboxPool(1667-1681)
🪛 ast-grep (0.40.0)
cli/commands/doctor_server.go
[warning] 201-203: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true, // Accept self-signed certs for diagnostics
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test-e2e
🔇 Additional comments (26)
cli/commands/doctor_server.go (6)
4-15: LGTM!The new imports support the HTTP endpoint checks and interactive recovery features.
43-48: LGTM!Capturing
connErrseparately enables detailed error checking in the interactive flow while maintaining connection validation logic.
57-61: LGTM!Standard pattern for extracting hostname from a host:port string.
69-78: LGTM!Clean endpoint health checks with uniform status reporting.
187-195: LGTM!Clean helper for uniform endpoint status reporting.
233-252: LGTM!The HTTP check correctly detects redirects (typically HTTP→HTTPS) by preventing redirect following and checking for 3xx status codes.
cli/commands/doctor_auth.go (1)
78-81: Good use of interactivity guard.Early return when non-interactive prevents prompts in CI/scripted environments.
pkg/ui/table.go (3)
262-267: LGTM - Correct approach for measuring width with OSC sequences.Stripping OSC sequences before passing to lipgloss.Width is the right approach since lipgloss handles ANSI color codes but not OSC 8.
357-360: Good extension to handle OSC sequences in styled cells.The check for
\x1b]alongside\x1b[correctly prevents runewidth.Truncate from mangling OSC 8 hyperlinks.
14-15: OSC 8 pattern is appropriate for current use case; BEL terminator support not required.XTerm accepts either BEL or ST for terminating OSC sequences, but the current pattern is sufficient for a display library. New code should always prefer ST due to standards compliance. The suggested regex change introduces issues: the negated character class
[^\x07\x1b]is overly restrictive and doesn't accurately represent OSC 8 format. For measuring display width and stripping sequences, the current pattern\x1b\]8;;[^\x1b]*\x1b\\is appropriate.clientconfig/clientconfig.go (1)
686-708: LGTM - Clean implementation following existing patterns.The method correctly mirrors the lookup order of
GetCluster(leaf configs first, then main), and the path construction aligns with howloadConfigDirsetsleafConfig.sourcePath(base name without extension at line 895).cli/commands/doctor.go (1)
207-218: LGTM - Clean integration of health counting.The health-driven messaging ("none", "X/Y unhealthy", "X deployed") provides clear, actionable status at a glance.
cli/commands/doctor_apps.go (3)
124-140: LGTM - Clean sandbox-based instance counting.Filtering by
RUNNINGstatus and mapping through version to app name is a sound approach for determining live instance counts.
260-261: Verifymirenbinary availability.The subprocess approach assumes the
mirenbinary is in PATH. If users run viago runor a renamed binary, these commands will fail. Consider usingos.Args[0]to reference the current executable.- cmd := exec.Command("miren", "logs", "-a", appName) + cmd := exec.Command(os.Args[0], "logs", "-a", appName) ... - cmd := exec.Command("miren", "deploy", "-a", appName) + cmd := exec.Command(os.Args[0], "deploy", "-a", appName)Also applies to: 270-271
222-241: Interactive mode is well-structured.Good use of the existing picker UI and proper exit handling with
[done]option. The guard forui.IsInteractive()prevents issues in non-TTY contexts.cli/commands/doctor_config.go (3)
19-26: LGTM - Clean helper function.The
isLocalClusterhelper correctly strips the port before checking if the address is local.
54-70: LGTM - Good cluster categorization and iteration.The cluster iteration with local/remote separation and source tracking provides clear organization for the display.
156-199: LGTM - Clean cluster config viewer.Good handling of the active cluster indicator suffix when extracting the cluster name.
cli/commands/login.go (2)
112-125: LGTM - Clean refactoring of login flow.The extraction of
LoginWithDefaultsas a public entry point and the internalloginhelper centralizes the logic nicely. This enables programmatic use fromDoctorAuthwhile maintaining the CLI option handling.
127-241: Parameter threading looks correct throughout the login flow.All references to the former option fields have been properly replaced with the function parameters (
cloudURL,identityName,keyName,noSave).cli/commands/app_list.go (6)
19-22: LGTM - OSC 8 hyperlink implementation.The escape sequence format is correct for terminal hyperlinks. This aligns with the UI enhancements mentioned in the PR summary.
28-46: Good error handling for missing configuration.Clean handling of the no-config case with helpful guidance, and proper support for cluster override.
122-139: LGTM - Correct most-recent deployment selection logic.The timestamp comparison correctly handles cases where parsing fails, preferring deployments with valid timestamps and the newest among valid ones.
286-295: LGTM - Route display with correct scheme detection.The scheme detection properly handles both
localhostand127.*addresses for HTTP, while using HTTPS for remote hosts. The URL uses the actual host while display shows the friendlierlocalhostreplacement.
156-182: LGTM - Pool state aggregation is well-structured.The per-app aggregation correctly sums instances across services and tracks cooldown state with crash count and remaining duration.
48-52: Good addition of resource cleanup.The
defer client.Close()ensures proper connection cleanup regardless of function exit path.
…to run and dont run it for them.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/commands/doctor.go (1)
65-73: Potential edge case with substring matching.Lines 67-68 use
strings.Containsto check for localhost/127.0.0.1, which could match hostnames like "my-localhost-server.com" or "10.127.0.0.1" and incorrectly prependhttp://instead ofhttps://.Consider using exact matching or prefix checking for more precision:
func normalizeAuthServerURL(authServer string) string { if !strings.HasPrefix(authServer, "http://") && !strings.HasPrefix(authServer, "https://") { - if strings.Contains(authServer, "localhost") || strings.Contains(authServer, "127.0.0.1") { + if authServer == "localhost" || strings.HasPrefix(authServer, "localhost:") || + authServer == "127.0.0.1" || strings.HasPrefix(authServer, "127.0.0.1:") { return "http://" + authServer } return "https://" + authServer } return authServer }
🧹 Nitpick comments (5)
cli/commands/doctor_server.go (2)
239-246: AddMinVersionto TLS configuration.The TLS config is missing
MinVersion. Even for diagnostic purposes, specifying a minimum TLS version prevents negotiation with older, insecure protocol versions.client := &http.Client{ Timeout: 5 * time.Second, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, // Accept self-signed certs for diagnostics }, }, }
258-270: Suspicious nested type assertion for*net.OpError.The inner type assertion checking if
opErr.Erris also a*net.OpErroris likely incorrect. TheErrfield of anet.OpErrortypically contains asyscall.Errnoor similar underlying error, not anotherOpError. This nested check is effectively dead code.Consider simplifying:
func describeHTTPError(err error) string { if netErr, ok := err.(net.Error); ok && netErr.Timeout() { return "timeout" } - // Check for connection refused - if opErr, ok := err.(*net.OpError); ok { - if sysErr, ok := opErr.Err.(*net.OpError); ok { - return sysErr.Err.Error() - } - return opErr.Err.Error() + // Unwrap to get underlying error message + var opErr *net.OpError + if errors.As(err, &opErr) { + if opErr.Err != nil { + return opErr.Err.Error() + } } return err.Error() }Using
errors.Asis more idiomatic for Go 1.13+ error handling and handles wrapped errors correctly.cli/commands/doctor_config.go (3)
77-109: Consider extracting table rendering logic.The local and remote cluster table rendering code (lines 77-92 and 94-109) is nearly identical. Consider extracting a helper function to reduce duplication:
func renderClusterTable(ctx *Context, clusters []clusterInfo, activeCluster string, label string) { if len(clusters) == 0 { return } ctx.Printf("\n%s\n", infoLabel.Render(label)) headers := []string{"CLUSTER", "ADDRESS", "SOURCE FILE"} var rows []ui.Row for _, c := range clusters { name := c.name if c.name == activeCluster { name = infoGreen.Render(c.name + "*") } rows = append(rows, ui.Row{name, c.cluster.Hostname, c.source}) } columns := ui.AutoSizeColumns(headers, rows, nil) table := ui.NewTable(ui.WithColumns(columns), ui.WithRows(rows)) ctx.Printf("%s\n", table.Render()) }
151-175: Consider handlingfmt.Scanlnerror.Line 173 ignores the error from
fmt.Scanln(). While this is likely acceptable for a pause mechanism in a CLI tool, consider at least logging or handling EOF/error conditions for better robustness.- fmt.Scanln() + if _, err := fmt.Scanln(); err != nil { + // Handle EOF or other errors gracefully + }
177-219: Consider handlingfmt.Scanlnerror.Line 217 has the same issue as line 173 - the error from
fmt.Scanln()is ignored. While acceptable for a pause mechanism, consider handling EOF or error conditions for better robustness.The overall logic for showing registration help based on existing registration status is well-designed and provides clear, actionable guidance.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cli/commands/commands.go(0 hunks)cli/commands/doctor.go(2 hunks)cli/commands/doctor_apps.go(0 hunks)cli/commands/doctor_auth.go(2 hunks)cli/commands/doctor_config.go(1 hunks)cli/commands/doctor_server.go(4 hunks)
💤 Files with no reviewable changes (2)
- cli/commands/commands.go
- cli/commands/doctor_apps.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/commands/doctor_auth.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
cli/commands/doctor_config.gocli/commands/doctor.gocli/commands/doctor_server.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: phinze
Repo: mirendev/runtime PR: 74
File: pkg/entity/cmd/schemagen/generator.go:0-0
Timestamp: 2025-06-03T00:19:24.104Z
Learning: User phinze prefers to file separate issues (like MIR-199) for code review findings that are outside the scope of the current PR rather than fixing them inline.
📚 Learning: 2025-10-09T21:31:23.219Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 238
File: cli/commands/server_install.go:256-275
Timestamp: 2025-10-09T21:31:23.219Z
Learning: In miren server uninstall flow (cli/commands/server_install.go), sending SIGUSR2 to start drain followed by systemctl stop is intentional: the stop signal (SIGTERM) allows the process to continue its cleanup, and the TimeoutStopSec=90s in the service file provides adequate time for the drain (which can take up to 5 minutes) to complete gracefully.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase doesn't actually return errors despite its signature. It starts servers in goroutines and immediately returns nil, handling any errors internally by logging them.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_server.go
📚 Learning: 2025-09-11T22:31:18.478Z
Learnt from: phinze
Repo: mirendev/runtime PR: 216
File: cli/commands/upgrade_linux.go:0-0
Timestamp: 2025-09-11T22:31:18.478Z
Learning: In cli/commands/upgrade_linux.go, the timeout handler intentionally kills only the child process with cmd.Process.Kill() rather than the entire process group with syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL). This is a deliberate design choice by the maintainers.
Applied to files:
cli/commands/doctor_config.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase has an error return type in its signature but always returns `nil`. It handles errors internally by logging them, making error checking at the call site unnecessary.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_server.go
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor.gocli/commands/doctor_server.go
📚 Learning: 2025-09-08T20:34:44.431Z
Learnt from: phinze
Repo: mirendev/runtime PR: 214
File: cli/commands/sandbox_list.go:3-10
Timestamp: 2025-09-08T20:34:44.431Z
Learning: In the miren.dev/runtime codebase, entity decoding is handled by methods like `sandbox.Decode(e.Entity())` on the generated protobuf structs, and status normalization is handled by `ui.CleanStatus()` function, so additional imports for encoding/json or strings are not needed for these operations.
Applied to files:
cli/commands/doctor.go
📚 Learning: 2025-06-02T20:03:41.073Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 81
File: cli/commands/entity_put.go:32-65
Timestamp: 2025-06-02T20:03:41.073Z
Learning: In cli/commands/entity_put.go and similar RPC client implementations, the user evanphx acknowledged that the unconditional rpc.WithSkipVerify in fallback scenarios needs security cleanup but deferred it to a future date.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-02-08T04:28:25.582Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 27
File: server/tls.go:19-24
Timestamp: 2025-02-08T04:28:25.582Z
Learning: For Go HTTP servers using http.Serve, error handling in the serving goroutine only needs to log errors, as the server can only fail when it's being closed. Complex error propagation or startup checks are unnecessary.
Applied to files:
cli/commands/doctor_server.go
🧬 Code graph analysis (3)
cli/commands/doctor_config.go (4)
clientconfig/clientconfig.go (2)
ClusterConfig(43-52)Config(55-74)cli/commands/config.go (1)
ConfigCentric(10-15)pkg/registration/registration.go (2)
LoadRegistration(261-277)StoredRegistration(80-96)pkg/ui/picker.go (3)
IsInteractive(360-371)PickerItem(13-18)RunPicker(337-357)
cli/commands/doctor.go (1)
pkg/auth/jwt.go (1)
Claims(43-47)
cli/commands/doctor_server.go (2)
clientconfig/clientconfig.go (1)
ClusterConfig(43-52)pkg/ui/picker.go (5)
IsInteractive(360-371)PickerItem(13-18)SimplePickerItem(21-24)RunPicker(337-357)WithTitle(294-298)
🪛 ast-grep (0.40.0)
cli/commands/doctor_server.go
[warning] 241-243: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true, // Accept self-signed certs for diagnostics
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-binaries (darwin, arm64, macos-latest)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test-e2e
🔇 Additional comments (15)
cli/commands/doctor_server.go (7)
1-14: LGTM!Imports are appropriate for the new HTTP endpoint checking and interactive functionality.
41-48: LGTM!Good improvement capturing the connection error in
connErrfor use in diagnostic messaging below. The error is properly utilized to provide context-specific suggestions.
138-161: LGTM!The interactive picker implementation is clean. Using the full help topic text as the ID works well for this use case since the items are static and don't require parsing.
166-185: LGTM!The help function provides clear, actionable guidance. The
fmt.Scanln()pause is acceptable here since this code path is only reached after theui.IsInteractive()check.
227-235: LGTM!Clean helper function for consistent endpoint status rendering.
272-292: LGTM!The HTTP check properly handles redirects (common for HTTPS upgrades) and provides meaningful status information.
80-136: LGTM!Good user experience providing context-specific suggestions. The local vs remote differentiation and error-message-based guidance help users quickly identify next steps. Listing available clusters for remote failures is a helpful touch.
cli/commands/doctor_config.go (5)
24-28: LGTM!The
clusterInfostruct is well-defined and serves its purpose of grouping cluster metadata for display.
42-48: LGTM!The graceful handling of missing configuration with clear getting-started instructions provides good user experience.
116-116: LGTM!The explicit error ignoring when loading registration is appropriate here, as the function is only used to conditionally adjust help text, and
LoadRegistrationreturnsnilwhen no registration exists.
118-149: LGTM!The interactive help flow is well-structured with appropriate exit conditions and clear user choices.
50-66: No issues found. TheGetClusterSourcemethod is properly implemented on the Config type and is correctly used at line 58.cli/commands/doctor.go (3)
82-132: LGTM! Clean authentication helper.The
tryAuthenticatefunction is well-structured with appropriate error handling for a diagnostic command. Silent failure on errors (returning default/empty results) aligns with the documented contract that "callers handle display."
194-199: LGTM! Improved authentication display.The authentication message now prefers the user's email from UserInfo when available, falling back to Claims.Subject. This provides a cleaner, more user-friendly display while maintaining a safe fallback.
236-236: LGTM! Help text aligns with simplified doctor command.The updated help text correctly references the focused topics (config, server, auth), consistent with the PR objective to streamline the doctor command by removing app/route counting and other ancillary information.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/commands/doctor_server.go (2)
183-183: Consider handling EOF infmt.Scanln().The
fmt.Scanln()calls will block indefinitely if stdin is closed. While the interactive guard reduces this risk, consider adding error handling or using a more robust input method likebufio.Reader.ReadString('\n')with EOF handling.Example pattern with error handling
reader := bufio.NewReader(os.Stdin) ctx.Printf("\n%s", infoGray.Render("Press Enter to continue...")) _, err := reader.ReadString('\n') if err != nil && err != io.EOF { // Handle error if needed } ctx.Printf("\n")Note: This would require adding
"bufio"and"io"imports.Also applies to: 201-201, 223-223
239-292: Consider extracting common HTTP client setup.The
checkHTTPSandcheckHTTPfunctions duplicate the client creation and request pattern. While the differences (TLS config, redirect handling) are valid, extracting a common helper could improve maintainability.Example refactoring:
func makeHTTPClient(timeout time.Duration, tlsConfig *tls.Config, checkRedirect func(*http.Request, []*http.Request) error) *http.Client { client := &http.Client{ Timeout: timeout, } if tlsConfig != nil { client.Transport = &http.Transport{ TLSClientConfig: tlsConfig, } } if checkRedirect != nil { client.CheckRedirect = checkRedirect } return client } func checkHTTPS(host string) (bool, string) { client := makeHTTPClient( 5*time.Second, &tls.Config{ MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, }, nil, ) // ... rest of function } func checkHTTP(host string) (bool, string) { client := makeHTTPClient( 5*time.Second, nil, func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }, ) // ... rest of function }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/doctor_server.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
cli/commands/doctor_server.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: phinze
Repo: mirendev/runtime PR: 74
File: pkg/entity/cmd/schemagen/generator.go:0-0
Timestamp: 2025-06-03T00:19:24.104Z
Learning: User phinze prefers to file separate issues (like MIR-199) for code review findings that are outside the scope of the current PR rather than fixing them inline.
📚 Learning: 2025-06-02T20:03:41.073Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 81
File: cli/commands/entity_put.go:32-65
Timestamp: 2025-06-02T20:03:41.073Z
Learning: In cli/commands/entity_put.go and similar RPC client implementations, the user evanphx acknowledged that the unconditional rpc.WithSkipVerify in fallback scenarios needs security cleanup but deferred it to a future date.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase doesn't actually return errors despite its signature. It starts servers in goroutines and immediately returns nil, handling any errors internally by logging them.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase has an error return type in its signature but always returns `nil`. It handles errors internally by logging them, making error checking at the call site unnecessary.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-10-09T21:31:23.219Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 238
File: cli/commands/server_install.go:256-275
Timestamp: 2025-10-09T21:31:23.219Z
Learning: In miren server uninstall flow (cli/commands/server_install.go), sending SIGUSR2 to start drain followed by systemctl stop is intentional: the stop signal (SIGTERM) allows the process to continue its cleanup, and the TimeoutStopSec=90s in the service file provides adequate time for the drain (which can take up to 5 minutes) to complete gracefully.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-02-08T04:28:25.582Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 27
File: server/tls.go:19-24
Timestamp: 2025-02-08T04:28:25.582Z
Learning: For Go HTTP servers using http.Serve, error handling in the serving goroutine only needs to log errors, as the server can only fail when it's being closed. Complex error propagation or startup checks are unnecessary.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/doctor_server.go
🪛 ast-grep (0.40.0)
cli/commands/doctor_server.go
[warning] 241-243: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true, // Accept self-signed certs for diagnostics
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test-e2e
🔇 Additional comments (4)
cli/commands/doctor_server.go (4)
43-48: LGTM: Connection error capture improves diagnostics.Preserving
connErrenables the enhanced error messaging shown later in the function, providing users with more actionable feedback.
72-78: LGTM: Endpoint checks enhance diagnostics.The HTTP/HTTPS endpoint checks provide valuable feedback about server reachability beyond just RPC connectivity.
138-161: LGTM: Interactive help picker improves user experience.The help picker provides a discoverable way for users to get guidance on common tasks, and the
ui.IsInteractive()guard ensures it only activates when appropriate.
227-235: LGTM: Clean helper function.The
printEndpointStatushelper provides consistent formatting for endpoint checks.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/commands/doctor_server.go (1)
92-106: String matching on error messages is fragile.Lines 92 and 98 use
strings.Containsto match error message text like "connection refused", "timeout", and "no recent network activity". This approach is fragile because:
- Error messages may be wrapped or formatted differently
- Different error implementations might phrase these differently
Consider using type assertions or
errors.Is/errors.Asto detect specific error types instead:var netErr net.Error if errors.As(connErr, &netErr) && netErr.Timeout() { // Handle timeout } var opErr *net.OpError if errors.As(connErr, &opErr) && opErr.Err != nil { if strings.Contains(opErr.Err.Error(), "connection refused") { // Handle connection refused } }Alternatively, check for
syscall.ECONNREFUSEDdirectly if available.cli/commands/doctor_config.go (1)
103-133: Interactive help flow is consistent and functional.The picker-based help interface matches the pattern established in
doctor_auth.go. Loading registration for contextual help is a nice touch, and ignoring the error is appropriate since it's optional information.Optionally, consider extracting the hard-coded path
/var/lib/miren/serverto a package-level constant for maintainability, since it appears multiple times across doctor commands.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cli/commands/doctor.go(3 hunks)cli/commands/doctor_auth.go(2 hunks)cli/commands/doctor_config.go(1 hunks)cli/commands/doctor_server.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
cli/commands/doctor_auth.gocli/commands/doctor_config.gocli/commands/doctor_server.gocli/commands/doctor.go
🧠 Learnings (9)
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/doctor_auth.gocli/commands/doctor_config.gocli/commands/doctor_server.gocli/commands/doctor.go
📚 Learning: 2025-10-09T21:31:23.219Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 238
File: cli/commands/server_install.go:256-275
Timestamp: 2025-10-09T21:31:23.219Z
Learning: In miren server uninstall flow (cli/commands/server_install.go), sending SIGUSR2 to start drain followed by systemctl stop is intentional: the stop signal (SIGTERM) allows the process to continue its cleanup, and the TimeoutStopSec=90s in the service file provides adequate time for the drain (which can take up to 5 minutes) to complete gracefully.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase doesn't actually return errors despite its signature. It starts servers in goroutines and immediately returns nil, handling any errors internally by logging them.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_server.go
📚 Learning: 2025-09-11T22:31:18.478Z
Learnt from: phinze
Repo: mirendev/runtime PR: 216
File: cli/commands/upgrade_linux.go:0-0
Timestamp: 2025-09-11T22:31:18.478Z
Learning: In cli/commands/upgrade_linux.go, the timeout handler intentionally kills only the child process with cmd.Process.Kill() rather than the entire process group with syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL). This is a deliberate design choice by the maintainers.
Applied to files:
cli/commands/doctor_config.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase has an error return type in its signature but always returns `nil`. It handles errors internally by logging them, making error checking at the call site unnecessary.
Applied to files:
cli/commands/doctor_config.gocli/commands/doctor_server.go
📚 Learning: 2025-02-08T04:28:25.582Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 27
File: server/tls.go:19-24
Timestamp: 2025-02-08T04:28:25.582Z
Learning: For Go HTTP servers using http.Serve, error handling in the serving goroutine only needs to log errors, as the server can only fail when it's being closed. Complex error propagation or startup checks are unnecessary.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-06-02T20:03:41.073Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 81
File: cli/commands/entity_put.go:32-65
Timestamp: 2025-06-02T20:03:41.073Z
Learning: In cli/commands/entity_put.go and similar RPC client implementations, the user evanphx acknowledged that the unconditional rpc.WithSkipVerify in fallback scenarios needs security cleanup but deferred it to a future date.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-11-05T18:29:33.602Z
Learnt from: phinze
Repo: mirendev/runtime PR: 294
File: components/activator/activator.go:450-450
Timestamp: 2025-11-05T18:29:33.602Z
Learning: In components/activator/activator.go and other files in the mirendev/runtime repository, when checking for `cond.ErrConflict` errors from optimistic concurrency control operations, use `errors.Is(err, cond.ErrConflict{})` rather than `errors.As`. The `cond.Conflict()` function returns value types, not pointers, so `errors.Is` is the correct pattern for detecting this error type.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-09-08T20:34:44.431Z
Learnt from: phinze
Repo: mirendev/runtime PR: 214
File: cli/commands/sandbox_list.go:3-10
Timestamp: 2025-09-08T20:34:44.431Z
Learning: In the miren.dev/runtime codebase, entity decoding is handled by methods like `sandbox.Decode(e.Entity())` on the generated protobuf structs, and status normalization is handled by `ui.CleanStatus()` function, so additional imports for encoding/json or strings are not needed for these operations.
Applied to files:
cli/commands/doctor.go
🧬 Code graph analysis (1)
cli/commands/doctor.go (1)
pkg/auth/jwt.go (1)
Claims(43-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test-e2e
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (14)
cli/commands/doctor_server.go (7)
3-14: LGTM!The new imports support the HTTP endpoint checks and interactive help features.
42-48: LGTM!Capturing the connection error enables more detailed diagnostics later in the function.
69-78: LGTM!The endpoint checks provide useful diagnostics with clear output formatting.
207-227: LGTM!The HTTPS check correctly enforces TLS 1.2 as the minimum version (line 214) while allowing self-signed certificates for diagnostic purposes. This addresses the previous review feedback.
241-261: LGTM!The HTTP check correctly prevents redirect following for diagnostic purposes, allowing you to detect if the server is configured to redirect HTTP to HTTPS.
82-82: No changes required. TheisLocalClusterfunction is defined incli/commands/doctor_config.goand is properly accessible fromdoctor_server.gosince both files are in the same package. Calling unexported functions across files within the same package is standard Go practice.Likely an incorrect or invalid review comment.
166-195: No issues found. The helper functionsprintHelpHeader,printCommand, andwaitForEnterare all properly defined incli/commands/doctor.go.cli/commands/doctor_auth.go (2)
78-105: LGTM: Interactive help flow is well-structured.The early return for non-interactive sessions and the picker loop with multiple exit conditions are appropriate. Users can view multiple help topics before exiting.
107-123: LGTM: Help functions provide clear guidance.Both helper functions provide actionable, step-by-step instructions for their respective tasks. The use of shared formatting utilities maintains consistency across the doctor commands.
cli/commands/doctor.go (1)
219-224: LGTM: Improved user display with email preference.The change to prefer email from
UserInfooverClaims.Subjectenhances user experience by showing a more recognizable identifier, with appropriate fallback logic.cli/commands/doctor_config.go (4)
14-21: LGTM: Proper hostname parsing with port handling.The function correctly strips the port component when present and delegates to
isLocalAddressfor the actual locality check.
23-27: LGTM: Clear data structure for cluster display.The struct appropriately groups cluster metadata needed for the table rendering logic.
41-102: LGTM: Well-structured cluster display with local/remote categorization.The refactored logic provides a cleaner, more informative view of configured clusters. The path shortening and table rendering improve readability, and the separation of local vs. remote clusters adds useful context.
135-181: LGTM: Contextual help provides clear, actionable guidance.Both helper functions effectively guide users through their respective workflows. The conditional logic in
showRegisterClusterHelpthat adapts based on existing registration state is particularly helpful, providing different instructions for initial registration vs. re-registration scenarios.
| func describeHTTPError(err error) string { | ||
| var netErr net.Error | ||
| if errors.As(err, &netErr) && netErr.Timeout() { | ||
| return "timeout" | ||
| } | ||
| var opErr *net.OpError | ||
| if errors.As(err, &opErr) { | ||
| return opErr.Err.Error() | ||
| } | ||
| return err.Error() | ||
| } |
There was a problem hiding this comment.
Add nil check for opErr.Err before dereferencing.
Line 236 accesses opErr.Err.Error() without checking if opErr.Err is nil. While uncommon, this could cause a panic.
🔎 Apply this diff to add defensive nil check:
func describeHTTPError(err error) string {
var netErr net.Error
if errors.As(err, &netErr) && netErr.Timeout() {
return "timeout"
}
var opErr *net.OpError
- if errors.As(err, &opErr) {
+ if errors.As(err, &opErr) && opErr.Err != nil {
return opErr.Err.Error()
}
return err.Error()
}📝 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.
| func describeHTTPError(err error) string { | |
| var netErr net.Error | |
| if errors.As(err, &netErr) && netErr.Timeout() { | |
| return "timeout" | |
| } | |
| var opErr *net.OpError | |
| if errors.As(err, &opErr) { | |
| return opErr.Err.Error() | |
| } | |
| return err.Error() | |
| } | |
| func describeHTTPError(err error) string { | |
| var netErr net.Error | |
| if errors.As(err, &netErr) && netErr.Timeout() { | |
| return "timeout" | |
| } | |
| var opErr *net.OpError | |
| if errors.As(err, &opErr) && opErr.Err != nil { | |
| return opErr.Err.Error() | |
| } | |
| return err.Error() | |
| } |
🤖 Prompt for AI Agents
In cli/commands/doctor_server.go around lines 229 to 239, the function
describeHTTPError dereferences opErr.Err without verifying it's non-nil; add a
defensive nil check: if errors.As(err, &opErr) { if opErr.Err != nil { return
opErr.Err.Error() } return opErr.Error() } so you return the inner error string
when present, otherwise fall back to the OpError message (and ultimately
err.Error()) to avoid a potential panic.
cli/commands/doctor.go
Outdated
| // waitForEnter prints "Press Enter to continue..." and waits for input | ||
| func waitForEnter(ctx *Context) { | ||
| ctx.Printf("\n%s", infoGray.Render("Press Enter to continue...")) | ||
| fmt.Scanln() | ||
| ctx.Printf("\n") | ||
| } |
There was a problem hiding this comment.
Consider handling errors from fmt.Scanln().
The function ignores errors from fmt.Scanln(), which could lead to unexpected behavior if stdin is closed or encounters errors. While acceptable for typical interactive use, handling or logging the error would make the behavior more predictable.
🔎 View suggested fix
func waitForEnter(ctx *Context) {
ctx.Printf("\n%s", infoGray.Render("Press Enter to continue..."))
- fmt.Scanln()
+ var input string
+ fmt.Scanln(&input)
ctx.Printf("\n")
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/commands/doctor.go around lines 27 to 32, waitForEnter currently ignores
the error returned by fmt.Scanln(); change it to capture the return error and
handle it — e.g. call _, err := fmt.Scanln(); if err != nil && err != io.EOF {
ctx.Printf("Warning: failed to read input: %v\n", err) } (import io if needed)
so stdin errors are logged while EOF is ignored for typical non-interactive
cases, then continue to print the trailing newline as before.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/commands/doctor_server.go (1)
237-240: Add nil check foropErr.Errbefore dereferencing.This issue was flagged in a previous review but remains unaddressed. Line 239 accesses
opErr.Err.Error()without checking ifopErr.Erris nil, which could cause a panic.🔎 Apply this diff to add defensive nil check:
var opErr *net.OpError -if errors.As(err, &opErr) { +if errors.As(err, &opErr) && opErr.Err != nil { return opErr.Err.Error() }
🧹 Nitpick comments (1)
cli/commands/doctor_server.go (1)
69-78: HTTP endpoint checks use hardcoded standard ports.The checks always test ports 443 and 80, but the cluster may be configured on a different port (e.g.,
hostname:8443). This could show failing endpoint checks even when the server is actually reachable on its configured port.Consider either:
- Adding a check for the actual configured port if it differs from 443
- Clarifying in the output that these are standard web port checks
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/doctor_server.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow standard Go formatting conventions
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Good comments should explain complex logic, document assumptions, or clarify non-obvious behavior rather than restating the code
Function/method comments should explain the purpose and any important side effects, not just restate the function name
Files:
cli/commands/doctor_server.go
🧠 Learnings (8)
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase doesn't actually return errors despite its signature. It starts servers in goroutines and immediately returns nil, handling any errors internally by logging them.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-05-22T05:27:15.270Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 57
File: cli/commands/dev.go:0-0
Timestamp: 2025-05-22T05:27:15.270Z
Learning: The `autotls.ServeTLS` function in the Miren runtime codebase has an error return type in its signature but always returns `nil`. It handles errors internally by logging them, making error checking at the call site unnecessary.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-10-09T21:31:23.219Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 238
File: cli/commands/server_install.go:256-275
Timestamp: 2025-10-09T21:31:23.219Z
Learning: In miren server uninstall flow (cli/commands/server_install.go), sending SIGUSR2 to start drain followed by systemctl stop is intentional: the stop signal (SIGTERM) allows the process to continue its cleanup, and the TimeoutStopSec=90s in the service file provides adequate time for the drain (which can take up to 5 minutes) to complete gracefully.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-02-08T04:28:25.582Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 27
File: server/tls.go:19-24
Timestamp: 2025-02-08T04:28:25.582Z
Learning: For Go HTTP servers using http.Serve, error handling in the serving goroutine only needs to log errors, as the server can only fail when it's being closed. Complex error propagation or startup checks are unnecessary.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-06-02T20:03:41.073Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 81
File: cli/commands/entity_put.go:32-65
Timestamp: 2025-06-02T20:03:41.073Z
Learning: In cli/commands/entity_put.go and similar RPC client implementations, the user evanphx acknowledged that the unconditional rpc.WithSkipVerify in fallback scenarios needs security cleanup but deferred it to a future date.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-02-05T23:40:16.698Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 24
File: cli/commands/dial_stdio.go:15-19
Timestamp: 2025-02-05T23:40:16.698Z
Learning: In CLI commands like `DialStdio`, errors should be explicitly written to stderr using `fmt.Fprintf(os.Stderr, ...)` rather than being returned, as these commands are run inline and need to provide immediate feedback to users.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-11-15T00:16:45.268Z
Learnt from: evanphx
Repo: mirendev/runtime PR: 373
File: components/activator/activator.go:1513-1513
Timestamp: 2025-11-15T00:16:45.268Z
Learning: In components/activator/activator.go and other files in the mirendev/runtime repository, when checking for not-found errors from Patch operations, use `errors.Is(err, cond.ErrNotFound{})`. The cond package passes NotFound errors through as value types, consistent with how `cond.ErrConflict{}` is handled for optimistic concurrency control operations.
Applied to files:
cli/commands/doctor_server.go
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/doctor_server.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test-e2e
🔇 Additional comments (7)
cli/commands/doctor_server.go (7)
3-14: LGTM!Imports are appropriate for the added HTTP/HTTPS endpoint checking and interactive UI functionality.
41-48: LGTM!Connectivity check properly captures the error for later diagnostic use while maintaining defensive nil checks.
80-136: LGTM!Good differentiation between local and remote server guidance. The string-based error matching is reasonable for user-facing diagnostics where the goal is helpful messaging rather than precise error handling.
138-166: LGTM!The interactive help flow is well-structured with proper exit conditions and clear menu options.
200-230: LGTM!Clean endpoint status formatting and well-configured HTTPS check with appropriate TLS settings for diagnostics (MinVersion enforced, InsecureSkipVerify acceptable for self-signed certs).
244-264: LGTM!The HTTP check correctly avoids following redirects and provides appropriate status messages for both redirect and direct response scenarios.
169-198: No issues found. All helper functions (printHelpHeader,printCommand,waitForEnter) are properly defined incli/commands/doctor.goand are accessible to the functions incli/commands/doctor_server.gowithin the same package.
evanphx
left a comment
evanphx
left a comment
There was a problem hiding this comment.
Let's not prompt them, I think we want to think through a proper guided setup rather than adding them into doctor.
cli/commands/doctor_auth.go
Outdated
| // Help picker | ||
| for { | ||
| items := []ui.PickerItem{ | ||
| ui.SimplePickerItem{Text: "How do I login with a different account?"}, | ||
| ui.SimplePickerItem{Text: "How do I add authentication to this server?"}, | ||
| ui.SimplePickerItem{Text: "[done]"}, | ||
| } | ||
|
|
||
| selected, err := ui.RunPicker(items, ui.WithTitle("Need help?")) | ||
| if err != nil || selected == nil || selected.ID() == "[done]" { | ||
| return nil | ||
| } | ||
|
|
||
| switch selected.ID() { | ||
| case "How do I login with a different account?": | ||
| showLoginDifferentAccountHelp(ctx) | ||
| case "How do I add authentication to this server?": | ||
| showAddAuthToServerHelp(ctx) | ||
| } | ||
| } |
There was a problem hiding this comment.
I still think that we want to do this as more of a guide setup later. So we should just display the question and then the output, no prompt.
cli/commands/doctor_config.go
Outdated
| // Help picker | ||
| for { | ||
| items := []ui.PickerItem{ | ||
| ui.SimplePickerItem{Text: "How do I add an existing cluster?"}, | ||
| ui.SimplePickerItem{Text: "How do I register a new cluster?"}, | ||
| ui.SimplePickerItem{Text: "[done]"}, | ||
| } | ||
|
|
||
| selected, err := ui.RunPicker(items, ui.WithTitle("Need help?")) | ||
| if err != nil || selected == nil || selected.ID() == "[done]" { | ||
| return nil | ||
| } | ||
|
|
||
| switch selected.ID() { | ||
| case "How do I add an existing cluster?": | ||
| showAddClusterHelp(ctx, cfg) | ||
| case "How do I register a new cluster?": | ||
| showRegisterClusterHelp(ctx, existing) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as the other, let's just display the questions and the answers rather than prompting them.
cli/commands/doctor_server.go
Outdated
| for { | ||
| items := []ui.PickerItem{ | ||
| ui.SimplePickerItem{Text: "How do I start a local server?"}, | ||
| ui.SimplePickerItem{Text: "How do I connect to a known remote server?"}, | ||
| ui.SimplePickerItem{Text: "How do I fix https/http connectivity?"}, | ||
| ui.SimplePickerItem{Text: "[done]"}, | ||
| } | ||
| protoLabel := infoGray.Render(fmt.Sprintf("/%s", p.proto)) | ||
| ctx.Printf(" %s %d%s %-6s %s\n", indicator, p.port, protoLabel, portStatus, infoGray.Render(p.desc)) | ||
|
|
||
| selected, err := ui.RunPicker(items, ui.WithTitle("Need help?")) | ||
| if err != nil || selected == nil || selected.ID() == "[done]" { | ||
| return nil | ||
| } | ||
|
|
||
| switch selected.ID() { | ||
| case "How do I start a local server?": | ||
| showStartLocalServerHelp(ctx) | ||
| case "How do I connect to a known remote server?": | ||
| showConnectRemoteServerHelp(ctx) | ||
| case "How do I fix https/http connectivity?": | ||
| showFixConnectivityHelp(ctx) | ||
| } | ||
| } |
Got it! Updated to just print the question and the commands without prompting the user. 
|
Miren Doctor now recoomends commands for users to run instead of running commands for them.
miren doctormiren doctor configmiren doctor authmiren doctor serverSummary by CodeRabbit
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.