Skip to content

fix: reject unsupported net-info output formats#3360

Open
haoshengzhen wants to merge 1 commit into
evstack:mainfrom
haoshengzhen:main
Open

fix: reject unsupported net-info output formats#3360
haoshengzhen wants to merge 1 commit into
evstack:mainfrom
haoshengzhen:main

Conversation

@haoshengzhen

@haoshengzhen haoshengzhen commented Jun 19, 2026

Copy link
Copy Markdown

Overview

This PR adds validation for the net-info --output flag.

Previously, net-info only handled json specially and silently fell back to text output for any other value. For example, --output yaml would still run the RPC requests and print text output, which is surprising for users and makes invalid CLI input harder to catch.

The command now accepts only text and json, and returns a clear error for unsupported output formats before making RPC calls.

Testing performed:

  • go test ./pkg/cmd -count=1
  • gofmt
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes
    • The net-info command now properly validates the --output flag and rejects unsupported output formats with a descriptive error message. Supported formats are text and json. Invalid format requests will now fail immediately with clear feedback.

Signed-off-by: haoshengzhen <haoshengzhen@outlook.com>
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

NetInfoCmd's RunE now reads the --output flag earlier and validates it via a switch, returning an error for formats other than text and json. The redundant later flag read is removed. A new test verifies that passing --output yaml returns an error with the expected message.

Changes

NetInfoCmd Output Format Validation

Layer / File(s) Summary
Early flag read and format validation
pkg/cmd/p2p.go, pkg/cmd/p2p_test.go
outputFormat is now read and validated at the start of RunE via a switch; unsupported values return an error. The duplicate GetString(flagOutput) call near the json branch is removed. A new test TestNetInfoCmd_InvalidOutputFormat confirms --output yaml fails with unsupported output format "yaml".

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐇 A rabbit checks its formats with care,
Only text and json are welcome there.
Pass yaml along? No, that won't do!
An error is returned, crisp and true.
Validate early, keep the code clean and fair! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: rejecting unsupported output formats for the net-info command.
Description check ✅ Passed The PR description provides clear context, explains the previous behavior, the rationale for the change, and includes testing performed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
pkg/cmd/p2p.go (1)

44-46: ⚡ Quick win

Wrap flag-read errors with context.

Line 45 returns the raw error; add context so failures are diagnosable from CLI output/logs.

Proposed patch
 		outputFormat, err := cmd.Flags().GetString(flagOutput)
 		if err != nil {
-			return err
+			return fmt.Errorf("read --%s flag: %w", flagOutput, err)
 		}

As per coding guidelines, "Wrap errors with context using fmt.Errorf in Go code".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cmd/p2p.go` around lines 44 - 46, In the error handling block around line
45 of the p2p.go command file, the raw error is being returned directly without
any context information. Replace the bare return statement with fmt.Errorf to
wrap the error and include a descriptive message about what operation failed
(e.g., reading flags, parsing configuration, etc.). This ensures that when the
error is displayed in CLI output or logs, it provides diagnostic context to help
users understand what went wrong.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/cmd/p2p.go`:
- Around line 44-46: In the error handling block around line 45 of the p2p.go
command file, the raw error is being returned directly without any context
information. Replace the bare return statement with fmt.Errorf to wrap the error
and include a descriptive message about what operation failed (e.g., reading
flags, parsing configuration, etc.). This ensures that when the error is
displayed in CLI output or logs, it provides diagnostic context to help users
understand what went wrong.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b9dc404a-3df4-4245-9bd2-63adf29e1831

📥 Commits

Reviewing files that changed from the base of the PR and between ec9f9bf and d5e030c.

📒 Files selected for processing (2)
  • pkg/cmd/p2p.go
  • pkg/cmd/p2p_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant