refactor(cli): stdlib-only CLI with LLM-friendly help and JSON config#188
Merged
Conversation
- Replace gopkg.in/yaml.v3 with encoding/json for config (config.json) - Auto-migrate legacy YAML configs to JSON on first load - Use text/tabwriter for table output, encoding/csv for CSV output - Inject io.Writer/io.Reader for full testability (Commands, REPL) - Structured --help with NAME/SYNOPSIS/DESCRIPTION/COMMANDS/FLAGS/EXAMPLES/ENVIRONMENT - Per-subcommand --help (e.g. mcp-trino query --help) - Unix exit codes: 0 success, 1 error, 2 usage error - Signal handling via signal.NotifyContext (SIGINT/SIGTERM) - Errors to stderr, data to stdout - No third-party library imports in cli package Co-authored-by: Claude <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review WalkthroughThis PR refactors the CLI to use injected io.Writers for stdout/stderr, centralizes CLI parsing/execution in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (runCLI)
participant Config as Config Loader
participant Commands as Commands
participant Trino as TrinoClient
participant Out as Writers (stdout/stderr)
rect rgba(200,230,255,0.5)
CLI->>Config: Load config (JSON-first / YAML fallback)
Config-->>CLI: CLIConfig
end
rect rgba(200,255,200,0.5)
CLI->>Commands: Build commands with writers (out, errOut) and context
CLI->>Commands: Dispatch subcommand / REPL
Commands->>Trino: Execute query / metadata calls
Trino-->>Commands: QueryResult / Error
Commands->>Out: Write formatted output (table/csv/json) or errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
internal/cli/output_test.go (1)
202-217: Make this test assert an observable behavior.The context is cancelled, but
erris discarded, so this passes whetherQueryhonors cancellation or not. Either use a mock that returnsctx.Err()and assert the failure, or stop cancelling the context and rename the test to what it actually verifies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/output_test.go` around lines 202 - 217, The test TestQueryExecution_OutputFormattingWithMockClient is discarding err and cancelling the context, so it doesn't assert behavior; either remove the early cancel and rename the test if you just want to verify output formatting, or make the mockTrinoClient simulate context cancellation (have its query implementation return ctx.Err()) and then call cmd.Query(ctx, "SELECT 1") and assert the returned error matches context.Canceled (or ctx.Err()); update expectations accordingly and keep references to TestQueryExecution_OutputFormattingWithMockClient, mockTrinoClient, newTestCommands, and cmd.Query when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cli.go`:
- Around line 241-249: The flag parse error branches for the "schemas"
(schemasFlagSet.Parse) and "tables" flag parsing currently return plain errors,
causing main to exit with code 1; change these to return usageError (the
sentinel/typed error used for usage exits) when flag parsing fails due to
invalid flags (e.g., unknown flag like --bogus). Concretely, in the block that
handles schemasFlagSet.Parse and the analogous tablesFlagSet.Parse branch,
detect parse failures from Parse and instead of returning fmt.Errorf("...: %w",
err) return usageError (or wrap the original err in usageError if your
convention requires it), preserving the hasFlags(commandArgs) fallback that
delegates to commands.Schemas when appropriate. Ensure both the schemas and
tables branches follow this same pattern.
- Around line 603-612: The code path that handles "config profile" currently
prints the usage when len(args) < 3 and returns nil, causing a successful (0)
exit for an incomplete command; change that to return a non-nil error after
printing usage so the CLI reports failure. Specifically, in the branch that
checks len(args) < 3 (uses args, w, and cliConfig.Current), keep the usage
output but replace the final "return nil" with a returned error (e.g.,
errors.New or fmt.Errorf) indicating a missing subcommand or invalid usage for
"config profile" so callers will exit non‑zero.
- Around line 123-126: The branch that handles explicit "--config ...yaml"
currently calls ParseYAMLConfigWithPath which only parses but leaves ConfigPath
pointing at the .yaml file; change this to run the YAML→JSON migration path
instead of the parse-only fallback: invoke the project's migration routine for
YAML configs (the function that converts and persists a canonical JSON config
and updates ConfigPath) rather than ParseYAMLConfigWithPath, and ensure after
migration ConfigPath points to the new config.json so subsequent commands (e.g.,
profile use) serialize to config.json not back to config.yaml.
In `@cmd/main.go`:
- Around line 67-72: The CLI exit handling is inconsistent: caller code
currently checks RunCLIMode() and maps *usageError to exitUsage but the
hasCLIOnlyFlags branch (and the path around lines 89-93) uses log.Fatalf,
producing a different exit code and message format; refactor so all CLI failures
funnel through a single helper (e.g., a new exitWithError(err error) used by
main and by branches) and change the hasCLIOnlyFlags branch and the other direct
log.Fatalf/Exit calls to either return a *usageError (so RunCLIMode returns it)
or call that helper; ensure the helper inspects errors with type assertion to
*usageError and exits with exitUsage for that type and exitError otherwise, and
use process-consistent message formatting (fmt.Fprintf to os.Stderr) instead of
log.Fatalf.
In `@internal/cli/commands.go`:
- Around line 277-279: The truncation summary is being written to c.out which
corrupts CSV streams; instead emit that metadata to the error stream. Change the
fmt.Fprintf call that handles queryResults.Truncated (currently writing to
c.out) to write to c.errOut (or, if the command tracks output format,
conditionally write the message to c.errOut only when the active format is
"csv"). Update the code path around queryResults.Truncated so CSV output remains
machine-parseable by moving the truncation message off stdout.
- Around line 90-93: The current prints using fmt.Fprintln and fmt.Fprintf
discard write errors (to c.out) which can fail with injected io.Writer; update
the code that iterates over catalogs to capture and propagate errors: check the
returned error from fmt.Fprintln(c.out, "Catalogs:") and from each
fmt.Fprintf(c.out, " - %s\n", catalog), and return or bubble that error from
the enclosing function (instead of ignoring it) so caller sees write failures;
use the existing c.out, catalogs and the surrounding function's error return to
locate where to add the error checks and early returns.
In `@internal/cli/config.go`:
- Around line 88-110: The YAML migration currently treats any unreadable or
malformed YAML as an empty config because parseSimpleYAML cannot fail; change
migrateYAMLConfig (and likewise ParseYAMLConfigWithPath) to detect and reject
malformed legacy YAML instead of silently defaulting: either update
parseSimpleYAML to return (CLIConfig, error) and propagate parse errors, or
after calling parseSimpleYAML check the raw file contents and parsed result
(e.g., if file is non-empty and parsed config equals zero-value or lacks
expected fields like Profiles/Current/Server) then return a descriptive error
rather than injecting defaults; ensure SaveCLIConfig is only called for
successfully parsed configs and update the callers of
parseSimpleYAML/ParseYAMLConfigWithPath accordingly.
In `@internal/cli/repl.go`:
- Around line 56-59: When Scan() returns false in the REPL loop (the block
containing if !r.scanner.Scan() { fmt.Fprintln(r.out); return nil }), check
r.scanner.Err() and return that error when non-nil instead of always returning
nil; if r.scanner.Err() is nil then treat it as EOF and return nil. Update the
code around r.scanner.Scan() to call err := r.scanner.Err() and handle err !=
nil by returning err, otherwise continue to print the newline and return nil.
Also ensure this logic is applied in the initial read path created by
NewREPLWithReader so real I/O failures are propagated.
- Around line 47-58: Writes to the injected io.Writer in repl.go (e.g., the
fmt.Fprintln/Fprint calls using r.out and r.prompt) currently ignore returned
errors; update the REPL to check each fmt.* return (and any fmt.Fprintf/Println
usages at the other occurrences) and propagate non-nil errors upstream (return
or wrap and return) instead of discarding them so write failures (broken pipe,
closed output) cause a proper error exit; specifically modify the write calls
around the greeting, prompt display, error/newline prints and any other
fmt.Fprint* uses in the REPL loop (referenced by r.out, r.prompt, r.scanner) to
handle and return the error.
---
Nitpick comments:
In `@internal/cli/output_test.go`:
- Around line 202-217: The test
TestQueryExecution_OutputFormattingWithMockClient is discarding err and
cancelling the context, so it doesn't assert behavior; either remove the early
cancel and rename the test if you just want to verify output formatting, or make
the mockTrinoClient simulate context cancellation (have its query implementation
return ctx.Err()) and then call cmd.Query(ctx, "SELECT 1") and assert the
returned error matches context.Canceled (or ctx.Err()); update expectations
accordingly and keep references to
TestQueryExecution_OutputFormattingWithMockClient, mockTrinoClient,
newTestCommands, and cmd.Query when making the change.
🪄 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: e8908f70-f1d9-4183-aeda-f9740d98f919
📒 Files selected for processing (11)
.gitignorecmd/cli.gocmd/main.gogo.modinternal/cli/commands.gointernal/cli/commands_test.gointernal/cli/config.gointernal/cli/config_test.gointernal/cli/output_test.gointernal/cli/repl.gointernal/cli/repl_test.go
Comment on lines
90
to
93
| fmt.Fprintln(c.out, "Catalogs:") | ||
| for _, catalog := range catalogs { | ||
| fmt.Printf(" - %s\n", catalog) | ||
| fmt.Fprintf(c.out, " - %s\n", catalog) | ||
| } |
There was a problem hiding this comment.
Return writer errors from the new output paths.
With io.Writer injection, these fmt.Fprint* calls can now fail, but this file still drops the errors. CI is already failing errcheck at this exact spot, and broken pipes will be reported as success unless the writes are propagated.
Suggested pattern
- fmt.Fprintln(c.out, "Catalogs:")
+ if _, err := fmt.Fprintln(c.out, "Catalogs:"); err != nil {
+ return fmt.Errorf("write catalogs header: %w", err)
+ }
for _, catalog := range catalogs {
- fmt.Fprintf(c.out, " - %s\n", catalog)
+ if _, err := fmt.Fprintf(c.out, " - %s\n", catalog); err != nil {
+ return fmt.Errorf("write catalog row: %w", err)
+ }
}📝 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
| fmt.Fprintln(c.out, "Catalogs:") | |
| for _, catalog := range catalogs { | |
| fmt.Printf(" - %s\n", catalog) | |
| fmt.Fprintf(c.out, " - %s\n", catalog) | |
| } | |
| if _, err := fmt.Fprintln(c.out, "Catalogs:"); err != nil { | |
| return fmt.Errorf("write catalogs header: %w", err) | |
| } | |
| for _, catalog := range catalogs { | |
| if _, err := fmt.Fprintf(c.out, " - %s\n", catalog); err != nil { | |
| return fmt.Errorf("write catalog row: %w", err) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Build & Verify Pipeline
[error] 90-90: golangci-lint (errcheck): Error return value of fmt.Fprintln is not checked.
🪛 GitHub Check: Code Quality
[failure] 92-92:
Error return value of fmt.Fprintf is not checked (errcheck)
[failure] 90-90:
Error return value of fmt.Fprintln is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/commands.go` around lines 90 - 93, The current prints using
fmt.Fprintln and fmt.Fprintf discard write errors (to c.out) which can fail with
injected io.Writer; update the code that iterates over catalogs to capture and
propagate errors: check the returned error from fmt.Fprintln(c.out, "Catalogs:")
and from each fmt.Fprintf(c.out, " - %s\n", catalog), and return or bubble that
error from the enclosing function (instead of ignoring it) so caller sees write
failures; use the existing c.out, catalogs and the surrounding function's error
return to locate where to add the error checks and early returns.
Comment on lines
47
to
58
| fmt.Fprintln(r.out, "mcp-trino CLI - Interactive Mode") | ||
| fmt.Fprintln(r.out, "Type '\\help' for help, '\\quit' or Ctrl-D to exit") | ||
| fmt.Fprintln(r.out) | ||
|
|
||
| history := []string{} | ||
|
|
||
| for { | ||
| // Display prompt | ||
| fmt.Print(r.prompt) | ||
| fmt.Fprint(r.out, r.prompt) | ||
|
|
||
| // Read input | ||
| if !r.scanner.Scan() { | ||
| // EOF (Ctrl-D) | ||
| fmt.Println() | ||
| fmt.Fprintln(r.out) | ||
| return nil |
There was a problem hiding this comment.
Propagate REPL write failures instead of ignoring them.
Now that output goes through an injected io.Writer, every fmt.Fprint* here is fallible. These ignored returns are what errcheck is flagging, and a broken pipe currently looks like a clean REPL exit.
Suggested pattern
- fmt.Fprint(r.out, r.prompt)
+ if _, err := fmt.Fprint(r.out, r.prompt); err != nil {
+ return fmt.Errorf("write prompt: %w", err)
+ }
@@
- fmt.Fprintf(r.out, "Current format: %s\n", r.commands.format)
+ if _, err := fmt.Fprintf(r.out, "Current format: %s\n", r.commands.format); err != nil {
+ return fmt.Errorf("write format message: %w", err)
+ }
return nilAlso applies to: 81-81, 103-106, 153-163, 200-227
🧰 Tools
🪛 GitHub Check: Code Quality
[failure] 54-54:
Error return value of fmt.Fprint is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/repl.go` around lines 47 - 58, Writes to the injected io.Writer
in repl.go (e.g., the fmt.Fprintln/Fprint calls using r.out and r.prompt)
currently ignore returned errors; update the REPL to check each fmt.* return
(and any fmt.Fprintf/Println usages at the other occurrences) and propagate
non-nil errors upstream (return or wrap and return) instead of discarding them
so write failures (broken pipe, closed output) cause a proper error exit;
specifically modify the write calls around the greeting, prompt display,
error/newline prints and any other fmt.Fprint* uses in the REPL loop (referenced
by r.out, r.prompt, r.scanner) to handle and return the error.
- Extract exitCLIError helper in main.go for consistent exit handling - YAML --config flag now migrates to JSON path, not parse-only - Return usageError for invalid schemas/tables flags (exit code 2) - config profile with no subcommand returns usageError - CSV truncation message goes to stderr (keeps stdout machine-parseable) - parseSimpleYAML returns error for malformed input - REPL checks Scanner.Err() before treating Scan()==false as EOF Co-authored-by: Claude <noreply@anthropic.com>
- Add gopkg.in/yaml.v3 as direct dep for config loading - Support both config.json and config.yaml natively (no forced migration) - Remove ~150 lines of fragile hand-rolled parsing code - SaveCLIConfig serializes in format matching ConfigPath extension - Legacy flat 'trino:' YAML auto-migrates to profiles via yaml.Unmarshal - Add dual struct tags (json + yaml) on all config types - Rewrite tests with dual-format coverage and roundtrip assertions Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/integration_test.go (1)
149-183: Integration tests still use YAML config files.The integration tests create YAML config files (e.g., line 155-162), which is valid since YAML is still supported as a fallback. However, consider adding a test with JSON config to validate the JSON-first behavior described in the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/integration_test.go` around lines 149 - 183, Test TestIntegration_ConfigFilePrecedence currently writes a YAML config (configContent) but the PR wants JSON-first behavior validated; update or add a test that also writes a JSON config file (e.g., configJSONContent) and invokes the built binary with "--config" pointing to the JSON file (or both files to assert JSON precedence) using the same tempDir/configFile pattern and the same exec.Command setup, then assert the output contains the host value from the JSON file (e.g., "from-json-host") while ensuring the YAML fallback case remains covered in TestIntegration_ConfigFilePrecedence or a new test; reference TestIntegration_ConfigFilePrecedence, configFile, configContent and the exec.Command invocation to locate where to add the JSON creation and assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/integration_test.go`:
- Around line 149-183: Test TestIntegration_ConfigFilePrecedence currently
writes a YAML config (configContent) but the PR wants JSON-first behavior
validated; update or add a test that also writes a JSON config file (e.g.,
configJSONContent) and invokes the built binary with "--config" pointing to the
JSON file (or both files to assert JSON precedence) using the same
tempDir/configFile pattern and the same exec.Command setup, then assert the
output contains the host value from the JSON file (e.g., "from-json-host") while
ensuring the YAML fallback case remains covered in
TestIntegration_ConfigFilePrecedence or a new test; reference
TestIntegration_ConfigFilePrecedence, configFile, configContent and the
exec.Command invocation to locate where to add the JSON creation and assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44b70eb8-fcb5-40c1-8386-90343ab85e49
📒 Files selected for processing (7)
cmd/cli.gocmd/integration_test.gocmd/main.gointernal/cli/commands.gointernal/cli/config.gointernal/cli/config_test.gointernal/cli/repl.go
✅ Files skipped from review due to trivial changes (1)
- cmd/main.go
- Test structured help sections (NAME/SYNOPSIS/DESCRIPTION/COMMANDS/FLAGS/EXAMPLES/ENVIRONMENT) - Test all subcommand --help outputs contain required sections - Test usageError returned for unknown commands, invalid format, missing args - Test --help and --version return nil error - Test REPL query errors go to stderr, not stdout - Test REPL meta-command errors go to stderr - Test REPL format command (set + display) - Test REPL EOF graceful exit - Coverage: internal/cli 81%->85%, cmd 9.5%->43.9% Co-authored-by: Claude <noreply@anthropic.com>
… exit codes - Document JSON + YAML config support with examples of both formats - Add Built-in Help section showing --help and per-subcommand help - Add Exit Codes table (0/1/2) - Update CLAUDE.md CLI Layer description with new architecture details - Fix docs/installation.md to use profiles format instead of legacy flat Co-authored-by: Claude <noreply@anthropic.com>
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
Changes
Testing
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation