Skip to content

fix: reject unsupported net-info output formats#3360

Open
haoshengzhen wants to merge 2 commits into
evstack:mainfrom
haoshengzhen:main
Open

fix: reject unsupported net-info output formats#3360
haoshengzhen wants to merge 2 commits 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 now fail immediately with clear feedback.
    • Added test coverage to ensure unsupported --output values (e.g., yaml) are rejected.
  • Documentation
    • Updated the Unreleased changelog to reflect the net-info --output validation fix.

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

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 08654512-defa-4ba0-9473-170e0d2c0e02

📥 Commits

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

📒 Files selected for processing (1)
  • CHANGELOG.md

📝 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. The changelog documents this fix.

Changes

NetInfoCmd Output Format Validation

Layer / File(s) Summary
Output format validation and test
pkg/cmd/p2p.go, pkg/cmd/p2p_test.go, CHANGELOG.md
outputFormat is now read and validated at the start of RunE via a switch that returns an error for unsupported values; 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". The changelog documents that unsupported formats are now rejected instead of falling back to text output.

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 clearly and specifically describes the main change: rejecting unsupported output formats for the net-info command, which aligns with the changeset.
Description check ✅ Passed The description provides clear context about the problem, solution, and testing performed, but lacks explicit link to the referenced issue (PR #3360 mentioned in CHANGELOG).
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

@julienrbrt

Copy link
Copy Markdown
Member

can you add a changelog?

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.95%. Comparing base (ec9f9bf) to head (d5e030c).

Files with missing lines Patch % Lines
pkg/cmd/p2p.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3360      +/-   ##
==========================================
- Coverage   60.97%   60.95%   -0.02%     
==========================================
  Files         129      129              
  Lines       14111    14115       +4     
==========================================
  Hits         8604     8604              
- Misses       4556     4558       +2     
- Partials      951      953       +2     
Flag Coverage Δ
combined 60.95% <71.42%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Copy link
Copy Markdown
Author

can you add a changelog?

Thank you for your approval. Added a changelog entry under Unreleased -> Fixed.

Please review it again. @julienrbrt

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.

2 participants