feat: add sh ports entity for listing listening ports#49
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a ports command to list listening ports using ss or netstat and a log command for file monitoring. Feedback suggests refactoring the ports logic to eliminate code duplication and moving the log feature to a separate PR as it is currently out of scope and lacks tests and shell completions.
| ports) | ||
| case "$1" in | ||
| match) | ||
| if [[ -z "$2" ]]; then | ||
| echo "Usage: u7 sh ports match <pattern>" | ||
| return 1 | ||
| fi | ||
| if command -v ss &>/dev/null; then | ||
| ss -tlnp | grep -i "$2" | ||
| else | ||
| netstat -tlnp 2>/dev/null | grep -i "$2" | ||
| fi | ||
| ;; | ||
| "") | ||
| if command -v ss &>/dev/null; then | ||
| ss -tlnp | ||
| else | ||
| netstat -tlnp 2>/dev/null | ||
| fi | ||
| ;; | ||
| *) echo "Usage: u7 sh ports [match <pattern>]" ; return 1 ;; | ||
| esac | ||
| ;; |
There was a problem hiding this comment.
The logic for choosing between ss and netstat is duplicated for the match case and the default case. You can refactor this to be more DRY (Don't Repeat Yourself) by extracting this logic into a helper function. This will improve the maintainability of the code. I've also added -- to grep as a good practice to prevent patterns starting with a hyphen from being interpreted as options.
ports)
_list_ports() {
if command -v ss &>/dev/null; then
ss -tlnp
else
netstat -tlnp 2>/dev/null
fi
}
case "$1" in
match)
if [[ -z "$2" ]]; then
echo "Usage: u7 sh ports match <pattern>"
return 1
fi
_list_ports | grep -i -- "$2"
;;
"")
_list_ports
;;
*) echo "Usage: u7 sh ports [match <pattern>]" ; return 1 ;;
esac
;;| log) | ||
| local file="$1" | ||
| if [[ -z "$file" ]]; then | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| fi | ||
| if [[ ! -f "$file" ]]; then | ||
| echo "File not found: $file" | ||
| return 1 | ||
| fi | ||
| case "$2" in | ||
| limit) | ||
| local n="${3:-20}" | ||
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: limit must be a positive integer, got '$n'" | ||
| return 1 | ||
| fi | ||
| tail -n "$n" "$file" | ||
| ;; | ||
| match) | ||
| if [[ -z "$3" ]]; then | ||
| echo "Usage: u7 sh log <file> match <pattern>" | ||
| return 1 | ||
| fi | ||
| grep -- "$3" "$file" | ||
| ;; | ||
| follow) | ||
| tail -f "$file" | ||
| ;; | ||
| "") | ||
| tail -n 20 "$file" | ||
| ;; | ||
| *) | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| ;; | ||
| esac | ||
| ;; |
There was a problem hiding this comment.
This pull request introduces the log entity, which seems unrelated to the main feature of adding the ports entity as described in the PR title and summary. Bundling unrelated changes in a single PR makes it harder to review and can complicate the git history.
The log feature also appears to be incomplete, as it's missing shell completions and tests. It's a best practice to keep pull requests focused on a single, cohesive change.
Please consider moving the log entity implementation and its related changes (e.g., help text) to a separate pull request where it can be completed and reviewed independently.
Greptile SummaryThis PR adds two new The implementation is straightforward and fits the existing codebase patterns well. A few minor polish items were found:
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/usability suggestions with no correctness or data-integrity impact. All three new findings are P2: a missing help-text entry, a silent-failure UX gap, and a grep case-sensitivity inconsistency. None affect correctness or security. Prior P1 issues (grep flag injection, unconditional test pass) were flagged in previous review rounds and are the author's call to address. The core feature works as intended. lib/show.sh — help text omission and silent fallback behaviour worth a quick fix before release. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["u7 sh ports"] --> B{subcommand?}
B -- "match <pattern>" --> C{pattern provided?}
C -- No --> D["echo Usage; return 1"]
C -- Yes --> E{ss available?}
E -- Yes --> F["ss -tlnp | grep -i <pattern>"]
E -- No --> G["netstat -tlnp 2>/dev/null | grep -i <pattern>"]
B -- "(empty)" --> H{ss available?}
H -- Yes --> I["ss -tlnp"]
H -- No --> J["netstat -tlnp 2>/dev/null"]
B -- "other" --> K["echo Usage; return 1"]
G -.->|"neither tool installed: silent exit 127"| L["no output, no error message"]
J -.->|"neither tool installed: silent exit 127"| L
Prompt To Fix All With AIThis is a comment left during a code review.
Path: lib/show.sh
Line: 363
Comment:
**`ports` entity missing from `--help` output**
The new `ports` entity is not listed in the `--help` help text. `port` (singular, line 363) is there, but the new `ports` (plural) command is absent. Users running `u7 sh --help` will have no indication it exists.
```suggestion
port <number>
ports [match <pattern>]
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: lib/show.sh
Line: 158-162
Comment:
**Silent failure when neither `ss` nor `netstat` is available**
When `ss` is absent and `netstat` is also missing, `netstat -tlnp 2>/dev/null` silently suppresses the "command not found" error and returns exit code 127. The user gets no output and no explanation. The same pattern exists in the empty-string branch (lines 165-169). Consider adding an explicit fallback error message:
```
if command -v ss &>/dev/null; then
ss -tlnp | grep -i "$2"
elif command -v netstat &>/dev/null; then
netstat -tlnp | grep -i "$2"
else
echo "Error: neither 'ss' nor 'netstat' is available" >&2
return 1
fi
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: lib/show.sh
Line: 159-162
Comment:
**Case-sensitivity inconsistency between `ports match` and `log match`**
`ports match` uses `grep -i` (case-insensitive), while the `log match` command added in the same PR uses plain `grep` (case-sensitive). It's not clear which is intentional. If case-insensitivity is desirable for ports, it should be consistent, or the difference should be documented.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix: guard sh ports tests for CI without..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "$verb" in | ||
| show|sh) | ||
| COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes port usage network git env http docker system definition functions --help" -- "$cur")) | ||
| COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes ports port usage network git env http docker system definition functions --help" -- "$cur")) |
There was a problem hiding this comment.
log entity missing from tab-completion word list
The log entity was added to show.sh in this PR but was not added to the entity completion word list. Running u7 sh <TAB> will not suggest log, making it effectively undiscoverable via tab completion.
| COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes ports port usage network git env http docker system definition functions --help" -- "$cur")) | |
| COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes ports port usage network git env log http docker system definition functions --help" -- "$cur")) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/completions.sh
Line: 5
Comment:
**`log` entity missing from tab-completion word list**
The `log` entity was added to `show.sh` in this PR but was not added to the entity completion word list. Running `u7 sh <TAB>` will not suggest `log`, making it effectively undiscoverable via tab completion.
```suggestion
COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes ports port usage network git env log http docker system definition functions --help" -- "$cur"))
```
How can I resolve this? If you propose a fix, please make it concise.| env) COMPREPLY=($(compgen -W "match" -- "$cur")) ;; | ||
| http) COMPREPLY=($(compgen -W "get head headers" -- "$cur")) ;; | ||
| docker) COMPREPLY=($(compgen -W "containers images volumes networks all" -- "$cur")) ;; | ||
| ports) COMPREPLY=($(compgen -W "match" -- "$cur")) ;; |
There was a problem hiding this comment.
log subcommand completion missing
ports got argument completion, but log (also added in this PR) has no completion case for its subcommands (limit, match, follow). Add a log case alongside the new ports case:
| ports) COMPREPLY=($(compgen -W "match" -- "$cur")) ;; | |
| ports) COMPREPLY=($(compgen -W "match" -- "$cur")) ;; | |
| log) COMPREPLY=($(compgen -W "limit match follow" -- "$cur")) ;; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/completions.sh
Line: 41
Comment:
**`log` subcommand completion missing**
`ports` got argument completion, but `log` (also added in this PR) has no completion case for its subcommands (`limit`, `match`, `follow`). Add a `log` case alongside the new `ports` case:
```suggestion
ports) COMPREPLY=($(compgen -W "match" -- "$cur")) ;;
log) COMPREPLY=($(compgen -W "limit match follow" -- "$cur")) ;;
```
How can I resolve this? If you propose a fix, please make it concise.| if command -v ss &>/dev/null; then | ||
| ss -tlnp | grep -i "$2" | ||
| else | ||
| netstat -tlnp 2>/dev/null | grep -i "$2" | ||
| fi |
There was a problem hiding this comment.
grep pattern not protected from flag injection
grep -i "$2" will interpret any pattern beginning with - as a grep flag (e.g. u7 sh ports match -v would invert the match silently). The log match handler added in the same PR correctly uses grep -- "$3" to terminate flag parsing. Apply the same protection here:
| if command -v ss &>/dev/null; then | |
| ss -tlnp | grep -i "$2" | |
| else | |
| netstat -tlnp 2>/dev/null | grep -i "$2" | |
| fi | |
| if command -v ss &>/dev/null; then | |
| ss -tlnp | grep -i -- "$2" | |
| else | |
| netstat -tlnp 2>/dev/null | grep -i -- "$2" | |
| fi |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/show.sh
Line: 158-162
Comment:
**`grep` pattern not protected from flag injection**
`grep -i "$2"` will interpret any pattern beginning with `-` as a grep flag (e.g. `u7 sh ports match -v` would invert the match silently). The `log match` handler added in the same PR correctly uses `grep -- "$3"` to terminate flag parsing. Apply the same protection here:
```suggestion
if command -v ss &>/dev/null; then
ss -tlnp | grep -i -- "$2"
else
netstat -tlnp 2>/dev/null | grep -i -- "$2"
fi
```
How can I resolve this? If you propose a fix, please make it concise.
| ports) | ||
| case "$1" in | ||
| match) | ||
| if [[ -z "$2" ]]; then | ||
| echo "Usage: u7 sh ports match <pattern>" | ||
| return 1 | ||
| fi | ||
| if command -v ss &>/dev/null; then | ||
| ss -tlnp | grep -i "$2" | ||
| else | ||
| netstat -tlnp 2>/dev/null | grep -i "$2" | ||
| fi | ||
| ;; | ||
| "") | ||
| if command -v ss &>/dev/null; then | ||
| ss -tlnp | ||
| else | ||
| netstat -tlnp 2>/dev/null | ||
| fi | ||
| ;; | ||
| *) echo "Usage: u7 sh ports [match <pattern>]" ; return 1 ;; | ||
| esac | ||
| ;; |
There was a problem hiding this comment.
The logic for choosing between ss and netstat is duplicated for both the plain ports command and ports match. You can refactor this to avoid repetition and improve maintainability by determining the base command first.
ports)
local list_cmd
if command -v ss &>/dev/null; then
list_cmd="ss -tlnp"
else
list_cmd="netstat -tlnp 2>/dev/null"
fi
case "$1" in
match)
if [[ -z "$2" ]]; then
echo "Usage: u7 sh ports match <pattern>"
return 1
fi
eval "$list_cmd" | grep -i -- "$2"
;;
"")
eval "$list_cmd"
;;
*) echo "Usage: u7 sh ports [match <pattern>]" ; return 1 ;;
esac
;;| log) | ||
| local file="$1" | ||
| if [[ -z "$file" ]]; then | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| fi | ||
| if [[ ! -f "$file" ]]; then | ||
| echo "File not found: $file" | ||
| return 1 | ||
| fi | ||
| case "$2" in | ||
| limit) | ||
| local n="${3:-20}" | ||
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: limit must be a positive integer, got '$n'" | ||
| return 1 | ||
| fi | ||
| tail -n "$n" "$file" | ||
| ;; | ||
| match) | ||
| if [[ -z "$3" ]]; then | ||
| echo "Usage: u7 sh log <file> match <pattern>" | ||
| return 1 | ||
| fi | ||
| grep -- "$3" "$file" | ||
| ;; | ||
| follow) | ||
| tail -f "$file" | ||
| ;; | ||
| "") | ||
| tail -n 20 "$file" | ||
| ;; | ||
| *) | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| ;; | ||
| esac | ||
| ;; |
There was a problem hiding this comment.
This pull request introduces a new log entity, which is unrelated to the stated goal of adding the ports entity. To maintain a clean and understandable commit history, it's best to keep pull requests focused on a single feature or bug fix. Please consider moving the implementation of the log entity to a separate pull request.
| network | ||
| git <authors|branches|tags|log [N]|status|diff|remotes> | ||
| env [match <pattern>] | ||
| log <file> [limit N|match <pattern>|follow] |
There was a problem hiding this comment.
This change is related to the log entity, which seems out of scope for this pull request. This should be moved to a separate PR along with the log entity's implementation.
1 participant
Summary
u7 sh portsentity to list all listening TCP ports usingss -tlnpwith fallback tonetstat -tlnpu7 sh ports match <pattern>to filter listening ports by pattern (grep)portsentity and itsmatchsubcommandTest plan
bash test.shpasses all 141 tests (0 failures)u7 sh portslists listening portsu7 sh ports match sshfilters outputu7 sh ports badargshows usage