Repository navigation
CLI and TUI: honest exit codes, opt-in history, safer text and exports - #555
Merged
Merged
Conversation
The three places that clean remote text for display (sanitize_for_terminal, the scan-probe banner cleaners and the CSV/Markdown cell cleaner) only tested char::is_control, so a right-to-left override or a zero-width character in a hostname, banner or TXT value passed through into text, CSV, Markdown and the SMB computer name. One shared predicate in netscli-core, is_unsafe_for_display, now covers the control characters plus the bidi controls, the zero-width characters, the rest of the invisible formatting block and the line and paragraph separators. The CLI exporters call it too (audit F8).
…d writes it ratatui drops control characters when it draws, so the screen never showed an escape sequence from an mDNS name or a TXT value. /export md reads the stored lines, not the screen, and wrote them to the file raw, so the sequence ran when someone cat'ed it. ratatui also keeps bidi and zero-width characters, so those were on screen too. Lines are now cleaned once when a command's output is stored, which covers the screen and both exports, and render_markdown cleans again because the file is the sink that outlives the session. The progress line is cleaned as well (audit F3, and the TUI part of F8).
Every discover, scan, inspect, sweep, dns, reverse and pcap run, from the CLI and the TUI, stored its whole result in ~/.netscli/netscli.db for ever, and every other command created the file too just by starting. Nothing reads it back, and the privacy page did not mention it. Recording is now opt-in. Without NETSCLI_HISTORY=1 no command opens or creates the database or its folder. With it on, behaviour is as before, and on Unix the folder is made 0700 and the file 0600. An existing database is left alone. The TUI no longer opens the database twice. With no home directory the database, and the TUI's default export folder, used to fall back to ./.netscli in the current directory. They now say there is no home instead of leaving a stray folder in a CI workspace (audit F6, F17, F18).
0 now means the command worked, 1 that it ran and failed, and 2 that the command line was wrong. Until now ping exited 0 when nothing answered, trace exited 0 when tracert or traceroute failed, doctor exited 0 with a missing dependency, and asking for two output formats exited 1 like a failed scan. - ping exits 1 when no reply came back, after printing the summary. - trace exits 1 when the tool exits non-zero (its own exit code was already stored and never looked at). Reaching the end of the route without an answer is still a success, because tracert and traceroute exit 0 for it. - doctor exits 1 when a required dependency is missing. DependencyStatus gains a required field for that, which is true only for libpcap in a build that has packet capture. Older state files read it as false. - The output flags are exclusive groups in clap, so --json --yaml is a usage error (2), and ping -c 0 is refused at parse time instead of reporting loss=0.0% for a ping that sent nothing. - Partial results, such as dns --record MX with no MX records, stay 0. The new tests run the real binary for the 0, 1 and 2 cases (audit F1).
netscli serve speaks MCP over stdin and stdout and exits when stdin closes. systemd starts a service with stdin on /dev/null, so the unit mcp-service --install wrote exited at once and Restart=always started it again every 5 seconds for as long as it was enabled. An MCP client starts the server itself. The command also wrote the unit on Windows and macOS, where nothing could read it. - --install writes nothing. It exits 1 and says why, with a pointer to the MCP setup guide. - --uninstall and --status stay, to find and remove the unit that earlier versions left behind. They look for it on every OS, because every OS got one. systemctl is only called on Linux. - Giving no flag, or two, is now a usage error (2). It used to print a hint and exit 0. - The unit renderer and its tests are gone (audit F7, and part of F26).
doctor on a standard build, which has no packet capture, printed a tick and installed: true for libpcap with the detail 'pcap support disabled at compile time'. The docs send people to doctor to learn what their build can do, and a tick said the opposite. It now reports libpcap as not installed, not required, and not compiled in, and still exits 0. setup would then have offered to install libpcap on such a build, which cannot help, so it now says there is nothing to install. setup --print is 'print only' and no longer writes ~/.netscli/config.json (audit F26).
…erminal Rows were coloured first and padded second. Rust pads by character count and the colour codes are characters, so an OPEN cell came out 5 columns wide instead of 14 and the rest of the row slid left. The header was aligned and the rows were not. Piped output and NO_COLOR output were always fine, and the tests only ever saw those, because colour is off when stdout is not a terminal. The test now forces colour on and checks that the visible text of the table is the same as the plain table (audit F2).
…rminal UI The CLI's tables only looked at NO_COLOR and whether stdout was a terminal. TERM=dumb still got colour codes and there was no way to keep colour through a pipe to less -R or into a CI log. The rules are now NO_COLOR first, then CLICOLOR_FORCE, then TERM=dumb, then the terminal check. The terminal UI ignored NO_COLOR and always drew in 24-bit colour. The finished frame is now stripped of colour when it is set, which keeps bold and shows the selected row in /config reversed instead of shaded. There is no --color flag. The environment variables cover it, and a flag is a new surface that can wait for someone to ask (audit F28).
/export split its arguments with a shell-style splitter, where a backslash escapes the next character. So /export --output C:\Users\me\out.md wrote to C:Usersmeout.md, a path relative to drive C, and a UNC path lost a backslash. A leading ~ was not expanded either, so ~/scans/out.md made a folder called ~ in the working directory. Arguments now split on whitespace with quotes grouping a value that has spaces in it, and a backslash is an ordinary character. A leading ~ means the home directory. The tests moved to their own file, which also had to happen for the file to stay under the size limit (audit F15).
The slash-command parsers skipped anything they did not understand. /ping host abc pinged four times, /mdns --timeout abc used 3000, /discover a b dropped the b, and /arp add 1.2.3.4 with no MAC showed the ARP table. Worst, /pcap eth0 --filter tcp port 80 captured with the filter tcp, which is every TCP packet, and said nothing. Each of those now answers with a usage error. /pcap reads quotes, so --filter "tcp port 80" works, and an unquoted filter gets an error that says to quote it. /mdns also takes --timeout-ms, the CLI's name for it, and /arp takes delete as well as del. /discover looked up every hostname with no way to turn it off, where the CLI, the desktop app, the MCP server and /sweep all default to not doing it. It now takes --resolve, off by default, and its summary says which it did. The catalog listed /sweep's no-op --no-resolve and not the --resolve that does something, and said /arp only shows the table. Both are corrected (audit F4, F11, F13).
- -j said the default is 256. When the terminal UI's /config has saved a different value, that is the default, and now the help says so. - mdns --type listed _http._tcp, but the mDNS library only accepts a type that ends in .local., so the example failed. The examples are written in full. - The completions example for PowerShell redirected into $PROFILE/netscli.ps1, but $PROFILE is a file. It now loads the script into the session. - --help lists the environment variables netscli reads: NETSCLI_HISTORY, NO_COLOR and CLICOLOR_FORCE (audit F19, F25).
… panics - Paste. Typed one key at a time, every pasted newline was a press of Enter, so pasting two lines ran the first as a command, and a clipboard with /arp clear on its second line ran that as well. On Linux and macOS the TUI now asks the terminal for bracketed paste, so a paste arrives as one event and goes into the input with its line breaks turned into spaces. On Windows crossterm cannot read it and a terminal asked for it would put the brackets into the input, so it stays as it was there. - Enter on a partly typed command. It ran the highlighted suggestion, so /e then Enter ran /export, which writes a file, when the user was heading for /exit, and /d started a discovery of the default subnet. It now finishes the name, as Tab does, and the next Enter runs it. - Panics. The default hook printed to the alternate screen, which was thrown away when the guard left it, so a crash looked like the TUI closing with exit code 101 and no reason. The terminal is now restored before the message is printed (audit F9, F12).
Compared with netscli inspect, /inspect printed the round-trip time as Rust debug output, (RTT: Some(3)) and (RTT: None), showed the MAC only when its maker was known, which drops the random addresses phones use, and never showed the host name. It now prints (3 ms), or nothing for a host that did not answer, shows any MAC with the maker where there is one, and has a Name line (audit F14).
The status line read HOSTNAME and then COMPUTERNAME. HOSTNAME is a shell variable that bash sets and zsh does not export, so on most Linux and macOS terminals the line said 'host n/a'. The operating system is now asked first and the variables are the fallback. sysinfo is already in the build through netscli-core, so this adds one line to Cargo.lock and no new package (audit F16).
netscli scan <name> resolves one address, the first A record or else the first AAAA, and threw it away. The text header showed only the name typed and the JSON has no address field, so for a round-robin or CDN name two runs could scan different machines with nothing to say which. The text header now reads 'example.com (93.184.216.34)'. The JSON shape is unchanged (audit F20).
netscli pcap writes capture.pcap in the working directory by default and truncated an existing file of that name without a word, so running it twice lost the first capture. It now stops with a message that names the file, and --force replaces it. The TUI's /export has always refused to overwrite a path the user gave (audit F21). Scripts that rerun a capture to the same name need --force added.
netscli scan 127.0.0.1 -p 1-4096 --json | head printed 'failed printing to stdout: The pipe has been ended' and a panic message, and exited 101, because println! panics when a write fails. The reader closing a pipe is it saying it has seen enough. The machine-readable formats and the large text reports (scan, discover, sweep, inspect, pcap --read) now go through one function that stops quietly with 0 on a closed pipe and still reports every other write error. The short prints elsewhere fit in a pipe's buffer, so a reader that leaves early never gets to them. The test hangs up on a half-megabyte scan of loopback and failed against the old code with the message above (audit F10).
- The discover table counted a cell's width in bytes and padded it in characters, and let a vendor run past its 24-character column, so a name with an accent, or a long vendor, moved the hostname column for that row. Cells are measured in characters and a long vendor is cut with an ellipsis. - 'Discovered 1 hosts' and '(1 hosts with open ports)' now say host. - mdns --timeout-ms 999999 waits 30 seconds, and then said nothing was found within 999999ms. It names the wait that happened (audit F29). The mDNS text moved into a function that returns the string, which is what lets a test see it. The three of them are covered, and so is a device that announces an escape sequence as its name, which had no test.
thiserror was declared by the CLI crate and never used. TuiApp.status was set in nine places and drawn nowhere. The 'press Esc again to exit' prompt is drawn from confirm_exit, and only a test ever read the string (audit F29).
The audit could not tell by reading whether a very small terminal panics, and asked for cases at 1x1, 10x3, 30x4 and 200x2. They are in, with a running command, suggestions and the exit prompt on top, and the TUI does not panic at any of them. The test does catch the failure it is for: turning one of the layout's saturating subtractions into a plain one makes it fail with 'attempt to subtract with overflow'.
commands.rs, the TUI command parsers and the TUI state tests each went over the 300-line limit with the tests added above. Same tests, new files: the repository already does this for cli_formatter and tui_export.
cli.md - The scan example was captured before 0.3.4 (filtered on loopback, no protocol). It is regenerated from this build against 127.0.0.1, and reads closed with a protocol and a latency. - New: exit codes, environment variables, local history and the limits that cut a value to fit. The mcp-service rows say what it does now. tui.md - The screenshot is an illustration, and the caption now says so and says what the real screen shows. - New: the keys, /arp add|del|clear, --resolve on /discover and /sweep, quoting for /pcap and /export, paste, NO_COLOR, and what is cleaned from network text. operations.md - Inspect with no ports checks 22, 80 and 443. It said host-only. - Exit codes for ping and trace, and the pcap output file. The crate README no longer claims native scrollback or --json on every subcommand (audit F22, F23, F24, M28).
run_server abandons its in-flight requests when stdin closes, but returning from main then drops the tokio runtime, and dropping a runtime waits for every blocking thread. A ping or capture already running on one cannot be abandoned, so it held the process open until its own timeout, which MCP allows to be ten minutes, with the client gone. The serve command now flushes stdout and exits with the server's result, 0 or 1 as before, instead of returning to main. The server's writer has flushed every response by the time run_server returns.
Contributor
|
Site preview: https://pr-555.netscli-site-preview.pages.dev Built from 6246965 with Production is unaffected: netscli.com is served from GitHub Pages via |
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.
Audit fixes for the CLI and terminal UI (
areas/cli-tui.md, local only). The DNS fallback is in #551.What changes for users
0means it worked,1that it ran and failed, and2that the command line was wrong. All three are documented in cli.md.pingwith no replies, a failedtrace, anddoctorwith a required dependency missing now exit 1.ping -c 0, exit 2 before anything runs.NETSCLI_HISTORY=1is set. Every scan used to be kept in~/.netscli/netscli.dbfor good, and nothing reads it back. Every command created the file just by starting. With the variable set, the folder and file are owner-only on Linux and macOS. An existing database is left alone.mcp-service --installno longer installs a systemd unit that could never work.serveis stdio-only, so the unit restarted every 5 seconds.--uninstalland--statusstay, to clean up a unit from an earlier version.netscli_core::is_unsafe_for_display, covers:/export mdcleans text before writing, so an mDNS name or TXT value can't plant an escape sequence in the file./pcap --filter "tcp port 80"keeps the whole filter. An unquoted multi-word filter is refused./discoverresolves names only with--resolve, like everywhere else./export.NO_COLOR.pcapneeds--forceto overwrite a capture.| head) exits quietly instead of panicking.serveexits as soon as its input closes.Product calls made conservatively (easy to revert)
/discoverin the TUI no longer resolves names by default.pcapneeds--forceto overwrite.doctor --jsongains arequiredfield.Checked (by the fixing agent, in its own target dir)
cargo fmt --all --check.cargo clippyon netscli and netscli-core, with and without--features pcap,-D warnings.cargo test:exit_codes6,broken_pipe1,mcp_stdio5scan --json | headwith exit 101. The new one exits 0.netscli.dbwas unchanged after scan, completions and doctor on the new binary.Follow-ups, after the other PRs land
dns/reverse.rs(desktop PR) and in the desktop app's CSV escaping, and add bidi cases totestdata/csv-escape.json.mcp-servicementions ininterface-coverage.md,desktop.md,mcp.mdandAGENTS.md.