Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CLI application to use the Cobra command framework and adds P2P ping functionality. The change replaces the custom dispatcher pattern with a structured command tree while introducing new P2P utilities for connecting to Supernode Kademlia endpoints.
- Replaces custom CLI dispatcher with Cobra command structure
- Adds P2P ping command with timeout support and secure connection handling
- Updates configuration system to support P2P endpoints and default to
~/.sncli/config.toml
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/sncli/main.go | Simplified main function to use Cobra's Execute() |
| cmd/sncli/cmd/*.go | New Cobra command implementations for root, p2p, ping, list, health-check, and get-status |
| cmd/sncli/cli/cli.go | Refactored CLI initialization and configuration handling |
| cmd/sncli/cli/p2p.go | New P2P functionality with secure connection and message encoding/decoding |
| cmd/sncli/cli/types.go | Updated configuration types to include P2P endpoint |
| p2p/kademlia/dht.go | Improved address parsing using net/url and net.SplitHostPort |
| cmd/sncli/go.mod | Updated dependencies to include Cobra and related libraries |
Comments suppressed due to low confidence (1)
cmd/sncli/cli/cmd_p2p_ping.go:1
- This validation is redundant since time.ParseDuration already handles negative durations and the check at line 22-24 ensures t is positive when using the default.
package cli
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "net" | ||
| "time" | ||
| "strconv" | ||
| "encoding/binary" |
There was a problem hiding this comment.
Remove trailing whitespace after the import statement.
| "encoding/binary" | |
| "encoding/binary" |
| ) | ||
|
|
||
| const ( | ||
| defaultMaxPayloadSize = 2 // MB |
There was a problem hiding this comment.
The magic number 2 should be defined with a more descriptive name or include units in the constant name, such as defaultMaxPayloadSizeMB.
| defaultMaxPayloadSize = 2 // MB | |
| defaultMaxPayloadSizeMB = 2 // MB |
| } | ||
| path = filepath.Join(home, path[1:]) | ||
| } | ||
| path = filepath.Clean(path) |
There was a problem hiding this comment.
Inconsistent indentation. This line uses spaces while other lines use tabs.
| path = filepath.Clean(path) | |
| path = filepath.Clean(path) |
| @@ -20,10 +18,22 @@ type CLIConfig struct { | |||
| Dir string `toml:"dir"` | |||
| KeyName string `toml:"key_name"` | |||
| LocalAddress string `toml:"local_address"` | |||
There was a problem hiding this comment.
Storing passphrases in plain text configuration (PassPlain field) poses a security risk. Consider removing this option or adding a warning comment about the security implications.
| LocalAddress string `toml:"local_address"` | |
| LocalAddress string `toml:"local_address"` | |
| // WARNING: Storing passphrases in plain text (PassPlain) is insecure and poses a security risk. | |
| // Avoid using this field in production environments. Prefer PassFile or PassEnv instead. |
|
|
||
| // If no specific service is requested, list all services | ||
| if len(c.opts.CommandArgs) == 0 || c.opts.CommandArgs[0] == "" { | ||
| if len(service) == 0 || service == "" { |
There was a problem hiding this comment.
The condition len(service) == 0 is redundant since service == \"\" already covers empty strings.
| if len(service) == 0 || service == "" { | |
| if service == "" { |
CLI: replace custom dispatcher with Cobra command tree
P2P: new command group
Config: