Trim comments that tell history or say the same thing twice

Comments that cited ADRs not in the repo, or described what the code used
to do, read as context a reviewer does not have. Several rationales were
written out in two to four places.

- ADR 003 and ADR 004 references go, as do "used to", "always has" and "predate this refactor"
- the group peer-check rationale lives on Server.trustSocket, the reload rationale on followControlConfig, the reserved-flag one on reservedFlags, the directory mode one on listen
- shorter headers on Request, execLine, Resolve, render and rebindLocked

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cmd/internal/ctl/ctl.go

@@ -295,7 +295,7 @@ func parseCommand(schema []commands.CommandSchema, argv []string) (req ctlReques
 // and then remembers which daemon they meant does not have to retype the line.
 //
 // Before the command word every flag is the client's, whatever it is, so an unknown one
-// is reported by fs as it always was, and -h is the client's usage. After it only the
+// is reported by fs, and -h is the client's usage. After it only the
 // flags fs defines are lifted out, and only those: the command's own flags are unknown
 // here, since they come from the daemon the line has yet to name, and -h after the
 // command asks for that command's help. Which command flags take a value is unknown too,

cmd/internal/ctl/ctl_test.go

@@ -82,9 +82,9 @@ func TestResolveSocketFlagWins(t *testing.T) {
 	assert.Equal(t, "/explicit.sock", p)
 }
 
-// TestParseCommandReportsWhatFlagSaid covers the three ways a flag line goes wrong. They
-// are different mistakes, and the old blanket "does not take" was wrong for two of them:
-// a valid flag with a missing value, and a valid flag with a value it cannot parse.
+// TestParseCommandReportsWhatFlagSaid covers the three ways a flag line goes wrong, which
+// are different mistakes: an unknown flag, a valid flag with a missing value, and a
+// valid flag with a value it cannot parse.
 func TestParseCommandReportsWhatFlagSaid(t *testing.T) {
 	schema := []commands.CommandSchema{{
 		Name: "change-remote",

commands.go

@@ -60,8 +60,8 @@ type certPEM struct {
 	PEM string `json:"pem"`
 }
 
-// Result shapes for print-relays. Field names are the JSON keys and predate this
-// refactor; changing them would change the output.
+// Result shapes for print-relays. Field names are the JSON keys; changing them would
+// change the output.
 type relayFor struct {
 	Error          string `json:",omitempty"`
 	Type           string
@@ -127,8 +127,8 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte
 	reg.Register(&commands.Command{
 		Name:             commands.Reload,
 		ShortDescription: "Reloads configuration from disk, same as sending HUP to the process",
-		// The line goes out before the reload, as it always has: a reload that restarts
-		// the ssh server ends the session that asked, and this is what it sees.
+		// The line goes out before the reload: a reload that restarts the ssh server ends
+		// the session that asked, and this is what it sees.
 		Run: func(fs any, a []string) (commands.Result, error) {
 			return commands.Did("Reloading config"), nil
 		},

commands/command.go

@@ -143,12 +143,10 @@ func isPresentationFlag(name string) bool {
 	return name == FlagJSON || name == FlagPretty
 }
 
-// execLine runs a command line against c, reporting problems to w. It is the path from
-// an ssh line to output: the line is parsed against the command's own FlagSet and Run
-// reads that FlagSet's struct directly. Nothing is serialised and parsed again on the
-// way, so a flag.Value whose String does not re-parse, flag.Func above all, is applied
-// as the line said and not as its String says. The control socket fills the same
-// FlagSet through Set in Invoke; Parse and Set are the flag package's one parser.
+// execLine runs an ssh command line against c, reporting problems to w. The line is
+// parsed into the command's own FlagSet and Run reads it directly, so a flag.Value whose
+// String does not re-parse, flag.Func above all, arrives as the line said. Invoke fills
+// the same FlagSet through Set.
 func (r *Registry) execLine(c *Command, args []string, w Output) error {
 	fl, fs, _ := commandFlags(c)
 
@@ -195,8 +193,7 @@ func (r *Registry) execLine(c *Command, args []string, w Output) error {
 
 // formatFor is the format a Result is rendered in. The line wins when it asked for one;
 // otherwise the surface that owns the registry decides, from the command and its
-// Result, which is how ssh keeps printing what it always has while another surface can
-// choose differently.
+// Result, so each surface can keep its own default.
 func (r *Registry) formatFor(c *Command, res Result, format Format) Format {
 	if format == FormatUnset {
 		return r.defaultFormatFor(c, res)

commands/registry.go

@@ -120,8 +120,8 @@ func WriteCommandHelp(c CommandSchema, w io.Writer) error {
 		}
 	}
 
-	// Same layout as flag.PrintDefaults, which is what this used to be, including its
-	// habit of leaving out a default that is the zero value.
+	// Same layout as flag.PrintDefaults, including its habit of leaving out a default
+	// that is the zero value.
 	for _, f := range c.Flags {
 		kind := ""
 		if !f.Bool {
@@ -321,11 +321,8 @@ type WithheldError struct {
 
 func (e *WithheldError) Error() string { return e.msg }
 
-// Resolve returns the command registered under name, or says why there is none. A
-// command this surface withholds is reported with the filter's reason, and one that
-// does not exist at all as unknown, so a client that misspells and one that asks for a
-// real command it cannot have here are told different things. It is the edge where a
-// name from the wire becomes a Command for a Request.
+// Resolve returns the command registered under name, or says why there is none: a
+// WithheldError with the filter's reason, or unknown for a name that does not exist.
 func (r *Registry) Resolve(name Name) (*Command, error) {
 	if c := r.Get(name); c != nil {
 		return c, nil

commands/registry_test.go

@@ -206,12 +206,8 @@ func TestDispatchReportsUnparseableLine(t *testing.T) {
 	assert.Contains(t, errOut.String(), "could not parse the command line")
 }
 
-// TestRegisterRejectsReservedFlags pins the contract that -json and -pretty belong to the
-// registry, not the command. commandFlags defines them on every command's FlagSet, so a
-// command that declared one itself would panic with "flag redefined" the first time an
-// operator ran it, on a goroutine with no recover. Better to hear about it at Register.
-// -config and -socket are nebula ctl's, taken off its line wherever they appear, so a
-// command that declared one would never see it from that surface.
+// TestRegisterRejectsReservedFlags: a command that declares a reserved flag is refused
+// at Register rather than when an operator first runs it; see reservedFlags.
 func TestRegisterRejectsReservedFlags(t *testing.T) {
 	for _, name := range []string{"json", "pretty", "h", "help", "config", "socket"} {
 		t.Run(name, func(t *testing.T) {

commands/render.go

@@ -41,10 +41,8 @@ const (
 	FlagPretty = "pretty"
 )
 
-// Flag names nebula ctl takes for itself, anywhere on its line: -config and -socket say
-// which daemon to talk to, and the client picks them out before it knows which command
-// the line names, let alone that command's flags. A command that declared either would
-// never receive it over that surface, so both are reserved.
+// Flag names nebula ctl takes for itself, anywhere on its line, to say which daemon to
+// talk to; see reservedFlags.
 const (
 	FlagConfig = "config"
 	FlagSocket = "socket"
@@ -55,14 +53,13 @@ const (
 // -pretty are added to every command's FlagSet by commandFlags, on every surface, so a
 // command that defined either would make that definition panic with "flag redefined"
 // on whichever goroutine ran the command; and -config and -socket are nebula ctl's own,
-// taken off the line before the command is parsed.
+// taken off the line before the client knows which command it names, so a command
+// that declared either would never receive it there.
 var reservedFlags = []string{"h", "help", FlagJSON, FlagPretty, FlagConfig, FlagSocket}
 
-// Render writes r in the requested format.
-//
-// This is the function ADR 004 means by "the same rendering functions": every surface
-// calls it, so a hostmap looks the same over ssh as it does through the control socket,
-// and a change to either rendering happens once.
+// Render writes r in the requested format. Every surface calls it, so a hostmap looks
+// the same over ssh as it does through the control socket, and a change to either
+// rendering happens once.
 func Render(c *Command, r Result, f Format, w io.Writer) error {
 	switch f {
 	case FormatJSON:
@@ -173,8 +170,8 @@ func (r Result) Empty() bool {
 }
 
 // Payload is what a program receives: the Value when there is one, otherwise the
-// Outcome. Neither surface ever sends the envelope itself, so print-tunnel -json emits
-// the tunnel and close-tunnel -json emits {ok, reason}, as both always have.
+// Outcome. No surface sends the envelope itself, so print-tunnel -json emits the tunnel
+// and close-tunnel -json emits {ok, reason}.
 func (r Result) Payload() any {
 	if r.Value != nil {
 		return r.Value

commands/request.go

@@ -10,12 +10,9 @@ import (
 // options by name in the form their flags parse, its operands, and the rendering the
 // line asked for, if any.
 //
-// It is where the wire meets the daemon. nebula ctl parses its line against the schema
-// and ParseLine turns what was set into one of these, which travels as a JSON object
-// and becomes a Request again on the control socket. An ssh line never becomes one: it
-// is parsed straight into the command's own FlagSet, which Run then reads, so there is
-// nothing to carry. Both surfaces drive Run through the FlagSet commandFlags builds,
-// ssh by Parse and the socket by Set, and the flag package keeps those two in step.
+// nebula ctl builds one with ParseLine and it travels as a JSON object to the control
+// socket. An ssh line never becomes one: it is parsed straight into the command's own
+// FlagSet; see execLine.
 type Request struct {
 	// Command is the command to run, resolved by the surface at its edge. See
 	// Registry.Resolve for the one that tells an unknown name from a withheld one.

commands/writer.go

@@ -5,8 +5,8 @@ import "io"
 // Output is where a command line surface's text goes.
 //
 // Out and Err are distinct so a command's primary output can stay machine readable on
-// a surface that separates the two. The ssh surface points both at the same channel,
-// which is why it sees a single merged stream exactly as it always has. The local
+// a surface that separates the two. The ssh surface points both at the same channel and
+// sees a single merged stream. The local
 // control socket does not use one at all: it invokes by named parameter and carries the
 // returned value, so there is no stream for a command to write to.
 type Output struct {

localapi/endpoint.go

@@ -51,12 +51,8 @@ func (e *endpoint) Path() string {
 }
 
 // rebindLocked brings the endpoint up at path with acc, moves it there, or, when it is
-// already there, applies a changed access. It is the one place a socket is installed and
-// the one way an endpoint comes up, so Wire's first attempt and its reload callback are
-// the same call, and an endpoint already at path is left as it is: Wire registers the
-// callback before the first attempt, so a HUP that lands between the two has the
-// callback do the first bind, and the first attempt then finds the socket it was about
-// to create already up.
+// already there, applies a changed access; with nothing changed it does nothing, which
+// is what lets Wire's first attempt and its reload callback be the same call.
 //
 // The new socket is bound before the old one is given up: a bad path in a reloaded
 // config leaves the operator with the endpoint they already had rather than none. serve

localapi/path.go

@@ -26,9 +26,7 @@ const CtlSocketKey = "local_api.control.socket"
 // CtlGroupKey is the optional config key naming a group that shares the control socket.
 // Unset, the socket admits the account nebula runs as and nobody else. Set, the socket
 // is handed to the group, 0660 on unix and an entry in the pipe's security descriptor on
-// windows, and the kernel's check on connect is the access control, as dockerd's -G
-// works; the daemon's own peer check stands aside, since on unix it could not tell a
-// member of the group from anyone else.
+// windows; see Access and Server.trustSocket.
 const CtlGroupKey = "local_api.control.group"
 
 // controlAccessFrom returns the control socket's access a config implies: the group

localapi/platform_unix.go

@@ -49,8 +49,9 @@ var currentUID = func() uint32 { return uint32(os.Geteuid()) }
 // socket file on every unix we build for. The control socket is 0600 and adds the peer
 // check the server applies on accept, unless a group shares it, in which case it is
 // 0660 with that group and the kernel's check is the whole of it; the info socket is
-// whatever the operator configured, since opening it to others is its purpose. The directory is not part of that, and is never
-// chmoded: a directory a service manager made for us is served from at whatever mode it
+// whatever the operator configured, since opening it to others is its purpose.
+//
+// The directory is not part of that, and is never chmoded: a directory a service manager made for us is served from at whatever mode it
 // came with (systemd's RuntimeDirectoryMode= is 0755 by default), and one an operator
 // pointed us into keeps whatever else it holds readable. The one we make ourselves is
 // 0755, searchable by everyone, so that a nebula ctl run as another user reaches the
@@ -69,11 +70,8 @@ func listen(path string, acc Access) (net.Listener, error) {
 	// exists, and an operator who names a deeper path creates it; a missing parent is a
 	// mistake to report, not a tree to build. A directory made here stays when the
 	// socket goes: an empty one costs nothing, and the next start wants it again.
-	//
-	// 0755 gives nothing away: the socket's own mode is the access control. What it
-	// gives is a directory others can look into, which is what a client that is not the
-	// daemon's user needs to reach the socket. Mkdir applies the umask, so the mode is
-	// pinned afterwards, as the socket's is; only on a directory this call created.
+	// Mkdir applies the umask, so the 0755 is pinned afterwards, as the socket's mode
+	// is; only on a directory this call created.
 	switch err := os.Mkdir(dir, 0755); {
 	case err == nil:
 		if err := os.Chmod(dir, 0755); err != nil {
@@ -283,7 +281,7 @@ func clearStale(path string) error {
 //
 // Only the node id names is removed. One that replaced it is another daemon's, bound
 // after ours was taken from under us, and is left where it is. A zero id, a node that
-// could not be identified when it was bound, is removed as it always was.
+// could not be identified when it was bound, is removed.
 func removeSocket(path string, id socketID) {
 	if cur, ok := identifySocket(path); ok && id != (socketID{}) && cur != id {
 		return

localapi/server.go

@@ -186,9 +186,8 @@ func (s *Server) handle(conn net.Conn) {
 		}
 	}()
 
-	// Where the platform can identify the caller, check it. Where it cannot, or where a
-	// group shares the socket, the endpoint is protected by the socket's mode and group
-	// or the pipe's security descriptor, which listen() set before serving.
+	// Where the platform can identify the caller, check it, unless a group shares the
+	// socket; see trustSocket. Otherwise listen's mode or security descriptor stands.
 	if peerCheckSupported && !s.trustSocket.Load() {
 		uid, err := peerUID(conn)
 		if err != nil {
@@ -308,10 +307,8 @@ func (s *Server) serve(req *Request) *Response {
 	return render(res, cmd, r, format, preface)
 }
 
-// render turns a command's Result into the response, in the format the caller asked for.
-// Both formats come from commands.Render, which is where ssh and the control socket
-// meet: the JSON a program reads here is byte for byte what -json prints at an ssh
-// prompt, and the text is the command's own rendering, which only the daemon can run.
+// render turns a command's Result into the response, in the format the caller asked for,
+// through commands.Render, so the JSON is byte for byte what -json prints over ssh.
 func render(res *Response, cmd *commands.Command, r commands.Result, format commands.Format, preface []byte) *Response {
 	buf := &bytes.Buffer{}
 	if err := commands.Render(cmd, r, format, buf); err != nil {

localapi/server_test.go

@@ -111,8 +111,8 @@ func TestServeAllowedCommand(t *testing.T) {
 	assert.Empty(t, errOut, "a command that worked reports no error")
 }
 
-// TestPanicInCommandDoesNotKillTheDaemon is ADR 003's requirement: a broken command
-// costs a connection, not the overlay.
+// TestPanicInCommandDoesNotKillTheDaemon: a broken command costs a connection, not the
+// overlay.
 func TestPanicInCommandDoesNotKillTheDaemon(t *testing.T) {
 	_, path := serve(t, basicRegistry())
 

localapi/wire.go

@@ -190,12 +190,9 @@ func reportRebind(l *slog.Logger, what, old, want string, err error) {
 // wireInfo brings up the info API when the config asks for one and follows the config
 // across reloads: enabling it, disabling it, moving its socket and changing its mode.
 //
-// Every reload is reconciled rather than gated on the section having changed. Rebind on
-// a server already where the config says is a no-op, so that costs nothing, and a bind
-// that failed for a reason outside the config, a runtime directory that was not there
-// yet, is retried by the plain reload that follows the fix, the same as the control
-// socket's. A section that does not parse keeps whatever is running: the operator is
-// told, and the programs relying on the endpoint are not cut off by a typo.
+// Every reload is reconciled, as the control socket's is; see followControlConfig. A
+// section that does not parse keeps whatever is running: the operator is told, and the
+// programs relying on the endpoint are not cut off by a typo.
 func wireInfo(ctx context.Context, l *slog.Logger, c *config.C, reg *commands.Registry) *InfoServer {
 	srv := NewInfoServer(l, reg)
 

sshd/session.go

@@ -48,19 +48,16 @@ func LogoutCommand(end func()) *commands.Command {
 }
 
 // DefaultFormat is what an ssh session renders when the operator gave neither -json nor
-// -pretty. It encodes what every command printed before rendering was shared: text
-// where a command has a human form, and compact JSON for the ones that never did
-// (print-tunnel, print-relays, query-lighthouse). It is ssh's policy, exported so a test
+// -pretty: text where a command has a human form, and compact JSON for the ones that
+// have none (print-tunnel, print-relays, query-lighthouse). It is ssh's policy, exported so a test
 // can dispatch through the same choice, and ssh's to change.
 func DefaultFormat(c *commands.Command, r commands.Result) commands.Format {
 	if !c.JSONOnly {
 		return commands.FormatText
 	}
 
-	// Backwards compatibility. The JSON-only commands always printed their refusals as
-	// a plain sentence ("No vpn address was provided") even though their data is JSON,
-	// so an outcome keeps rendering as text here. Drop this branch to make ssh's default
-	// apply uniformly; the output golden will show exactly which lines change.
+	// A JSON-only command's refusal ("No vpn address was provided") is a sentence for a
+	// person, so an outcome renders as text here; ssh users and scripts expect it.
 	if r.Value == nil {
 		return commands.FormatText
 	}