Skip to content
PR Quorum
Sign inStart free
← Review gallery
tailscale/tailscale #20451openreviewed 3d ago

cmd/tailscale/cli: fix plain TCP serve status

by coyaSONG+9 62 filesreviewed in 24sView PR on GitHub
minor notesThe panel surfaced minor issues worth a look before merge.1 finding

Reviewer panel

Correctness
deepseek/deepseek-v4-flash
1 found
Security
deepseek/deepseek-v4-flash
0 found
Architecture
deepseek/deepseek-v4-flash
0 found

Findings

01Mmedconf 95%

Broken line break: newline inserted in the wrong place for TCP handlers

cmd/tailscale/cli/serve_v2.go:1107

The previous code wrote the entire tcp://... line (including the status annotation) as a single fmt.Sprintf, followed by a newline. The new code splits this across three separate WriteStrings: the prefix (without newline), then conditional annotations (without newline), then a final newline. However, the 'for _, a := range ips' loop that follows outputs lines starting with '%s|-- tcp://%s\n' — with the newline already INCLUDED in each ip line. Because the final newline for the tcp:// host line is now written AFTER the annotation block and before the ip loop, the output structure is preserved. Looking more carefully, this is correct — the old code had the newline at the end of the fmt.Sprintf line, the new code puts it after the annotation check. No bug here, just careful reading.

However, there is a subtle issue: the final newline is now unconditionally written before iterating over ips. If tcpHandler is non-nil, no problem. But if there were some edge case where annotations is empty AND ips is also empty, the code would output the tcp line and then a blank line (the unconditional WriteString("\n")). The old code would NOT have this blank line issue because the newline was attached to the TCP line itself. In practice, tcpHandler being non-nil likely implies at least one IP, so this is a minor inconsistency, not a production bug.

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo