Render every command through Run/Text

cmd/internal/ctl/ctl.go

@@ -1,6 +1,7 @@
 package ctl
 
 import (
+	"encoding/json"
 	"errors"
 	"flag"
 	"fmt"
@@ -62,7 +63,7 @@ func Trailing(rest []string, configPath string) (code int, done bool) {
 	if configPath != "" {
 		// First, so a -config on the ctl side of the line still wins: Parse applies
 		// flags in order and the last value set is the one read.
-		args = append([]string{"-config", configPath}, args...)
+		args = append([]string{"-" + commands.FlagConfig, configPath}, args...)
 	}
 
 	return Run(args), true
@@ -83,8 +84,8 @@ func Run(args []string) int {
 	fs := flag.NewFlagSet("nebula ctl", flag.ContinueOnError)
 	fs.SetOutput(os.Stderr)
 
-	configPath := fs.String("config", "", "Path to the config of the running nebula, used to locate its control socket")
-	socketPath := fs.String("socket", "", "Path to the control socket, instead of the one the config implies")
+	configPath := fs.String(commands.FlagConfig, "", "Path to the config of the running nebula, used to locate its control socket")
+	socketPath := fs.String(commands.FlagSocket, "", "Path to the control socket, instead of the one the config implies")
 	fs.Usage = func() {
 		fmt.Fprintf(os.Stderr, "Usage: nebula ctl [flags] <command> [command flags]\n\n")
 		fmt.Fprintf(os.Stderr, "Inspects and controls a nebula already running on this machine.\n")
@@ -162,22 +163,13 @@ func Run(args []string) int {
 		return 0
 	}
 
-	res, err := client.Invoke(string(req.name), req.Params, req.Args)
+	res, err := client.Invoke(string(req.name), req.Params, req.Args, wireFormat(req.Format))
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "nebula: %s\n", err)
 		return 1
 	}
 
-	out := localapi.Text(res.Result)
-
-	// Indentation is the client's business: whitespace does not survive a JSON envelope,
-	// and the daemon has no reason to care how its answer is laid out.
-	if req.Params["pretty"] == "true" {
-		if indented, ok := localapi.Pretty(res.Result); ok {
-			out = indented
-		}
-	}
-
+	out := output(res.Result, req.Format)
 	if out != "" {
 		fmt.Fprint(os.Stdout, out)
 		if !strings.HasSuffix(out, "\n") {
@@ -185,30 +177,71 @@ func Run(args []string) int {
 		}
 	}
 
+	// Error set is the one failure signal, whether the request never ran or the command
+	// ran and refused. A refusal asked for in JSON has already gone to stdout as the
+	// {ok, reason} object, and is not said again on stderr; the exit status carries it.
 	if res.Error == "" {
 		return 0
 	}
 
-	msg := res.Error
-	if !strings.HasSuffix(msg, "\n") {
-		msg += "\n"
+	if len(res.Result) == 0 {
+		msg := res.Error
+		if !strings.HasSuffix(msg, "\n") {
+			msg += "\n"
+		}
+		fmt.Fprint(os.Stderr, msg)
 	}
-	fmt.Fprint(os.Stderr, msg)
 
 	return 1
 }
 
+// output renders what the daemon answered for the format the line asked for. A text
+// request is answered with the command's rendering carried as a JSON string, and is
+// unquoted back into it; a JSON request is answered with the value itself, which is
+// printed as it came, since a value that happens to be a string is still JSON to the
+// script reading it. Indentation is the client's business: whitespace does not survive a
+// JSON envelope, and the daemon has no reason to care how its answer is laid out.
+func output(result json.RawMessage, format commands.Format) string {
+	switch format {
+	case commands.FormatJSON:
+		return string(result)
+	case commands.FormatPrettyJSON:
+		if indented, ok := localapi.Pretty(result); ok {
+			return indented
+		}
+		return string(result)
+	default:
+		return localapi.Text(result)
+	}
+}
+
 // ctlRequest is one parsed nebula ctl command line: the command's name, which only the
 // daemon can resolve, and the line as every surface parses it. The Request's Command is
-// nil here, since the CLI holds a schema rather than the command.
+// nil here, since the CLI holds a schema rather than the command, and its Format is
+// FormatUnset when the line named none, which to nebula ctl means text: its default
+// differs from the API's.
 type ctlRequest struct {
 	name commands.Name
 	commands.Request
 }
 
+// wireFormat is what a request asks the daemon for. Pretty is the client's business,
+// since whitespace does not survive a JSON envelope, so it travels as json. No
+// preference is text, nebula ctl's default: a person gets the human rendering wherever
+// one exists.
+func wireFormat(f commands.Format) string {
+	switch f {
+	case commands.FormatJSON, commands.FormatPrettyJSON:
+		return "json"
+	default:
+		return "text"
+	}
+}
+
 // parseCommand turns the user's command line into a ctlRequest, using the schema the
 // daemon supplied. The line is parsed with the same rules the daemon applies to an ssh
-// line, through ParseLine, so a line means the same on both.
+// line, through ParseArgs, and ParseLine reads what was set into the Request to send, so
+// a line means the same on both.
 //
 // printedHelp reports that help was written for the command instead, and there is
 // nothing to send.
@@ -230,11 +263,6 @@ func parseCommand(schema []commands.CommandSchema, argv []string) (req ctlReques
 		return ctlRequest{name: name, Request: commands.Request{Args: argv[1:]}}, false, nil
 	}
 
-	if commands.IsHelpRequest(argv[1:]) {
-		printCommandHelp(found)
-		return ctlRequest{}, true, nil
-	}
-
 	// The flag package's own report is a bare message followed by a usage dump in a layout
 	// that does not match WriteCommandHelp, so it is silenced and the error is reported
 	// once, below, with a pointer at the help this command actually prints.
@@ -249,15 +277,29 @@ func parseCommand(schema []commands.CommandSchema, argv []string) (req ctlReques
 		fs.String(f.Name, f.Default, f.Usage)
 	}
 
-	if err := fs.Parse(argv[1:]); err != nil {
+	operands, err := commands.ParseArgs(fs, argv[1:])
+	if err != nil {
+		if errors.Is(err, flag.ErrHelp) {
+			// The same rendering the daemon uses for `help <command>`, so -h and help agree.
+			_ = commands.WriteCommandHelp(*found, os.Stdout)
+			return ctlRequest{}, true, nil
+		}
+
 		// Say what flag did: an unknown flag, a missing value and a malformed value are
 		// different mistakes, and "does not take" would be wrong for two of them.
 		return ctlRequest{}, false, fmt.Errorf("nebula: %s: %s (see 'nebula ctl %s -h')", name, err, name)
 	}
 
 	// Only what the user actually set travels. A parameter left alone is the daemon's
-	// default to apply, not the client's to assert.
-	return ctlRequest{name: name, Request: commands.ParseLine(fs, fs.Args())}, false, nil
+	// default to apply, not the client's to assert, and the presentation flags are the
+	// client's to interpret, never sent.
+	//
+	// Where the command has no human form the daemon says so, and only once the command
+	// has run: a refusal ("No vpn address was provided") always has a human form, so
+	// telling the user to pass -json before finding out whether the command would even
+	// accept the line sends them to fix the wrong thing. That is why the schema does not
+	// say which commands have a human form: nothing should decide on it before running.
+	return ctlRequest{name: name, Request: commands.ParseLine(fs, operands)}, false, nil
 }
 
 // splitLine divides a nebula ctl line into the client's own flags, for fs to parse, and
@@ -271,10 +313,10 @@ func parseCommand(schema []commands.CommandSchema, argv []string) (req ctlReques
 // 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,
-// so the client's names are its alone: a command must not give one a value here. A "--"
-// before the command is consumed, as fs.Parse would have; one after it belongs to the
-// command line and ends the lifting, so an operand spelled like a client flag can still
-// be passed.
+// so the client's names are reserved in the commands package and no command can give
+// one a value that would be lifted from here. A "--" before the command is consumed, as
+// fs.Parse would have; one after it belongs to the command line and ends the lifting,
+// so an operand spelled like a client flag can still be passed.
 func splitLine(fs *flag.FlagSet, args []string) (own, cmd []string) {
 	seenCommand := false
 	for i := 0; i < len(args); i++ {
@@ -296,7 +338,7 @@ func splitLine(fs *flag.FlagSet, args []string) (own, cmd []string) {
 			}
 
 			own = append(own, a)
-			if f, hasValue := ownFlag(fs, a); f != nil && !hasValue && takesValue(f) && i+1 < len(args) {
+			if f, hasValue := commands.LookupArg(fs, a); f != nil && !hasValue && !commands.IsBoolFlag(f) && i+1 < len(args) {
 				i++
 				own = append(own, args[i])
 			}
@@ -304,9 +346,11 @@ func splitLine(fs *flag.FlagSet, args []string) (own, cmd []string) {
 		}
 
 		if len(a) >= 2 && a[0] == '-' {
-			if f, hasValue := ownFlag(fs, a); f != nil {
+			// The same reading of the argument ParseArgs makes, so a word this lifts as a
+			// client flag's value is one the daemon's parser would have read the same way.
+			if f, hasValue := commands.LookupArg(fs, a); f != nil {
 				own = append(own, a)
-				if !hasValue && takesValue(f) && i+1 < len(args) {
+				if !hasValue && !commands.IsBoolFlag(f) && i+1 < len(args) {
 					i++
 					own = append(own, args[i])
 				}
@@ -320,23 +364,6 @@ func splitLine(fs *flag.FlagSet, args []string) (own, cmd []string) {
 	return own, cmd
 }
 
-// ownFlag returns the flag of fs that the argument a names, nil when it names none, and
-// whether a carries its value after an "=".
-func ownFlag(fs *flag.FlagSet, a string) (f *flag.Flag, hasValue bool) {
-	name := strings.TrimPrefix(strings.TrimPrefix(a, "-"), "-")
-	if eq := strings.IndexByte(name, '='); eq >= 0 {
-		name, hasValue = name[:eq], true
-	}
-	return fs.Lookup(name), hasValue
-}
-
-// takesValue reports whether f reads the argument after it as its value, which every
-// flag does except a boolean.
-func takesValue(f *flag.Flag) bool {
-	bf, ok := f.Value.(interface{ IsBoolFlag() bool })
-	return !ok || !bf.IsBoolFlag()
-}
-
 // resolveSocket decides which socket to talk to. It never searches: the answer is the
 // socket the operator named, or the one the config implies, and nothing else.
 //
@@ -403,22 +430,3 @@ func socketFromConfigFile(configPath string, named bool) (path, readConfig, over
 	}
 	return p, configPath, overrideIn, nil
 }
-
-func printCommandHelp(c *commands.CommandSchema) {
-	fmt.Fprintf(os.Stdout, "%s - %s\n", c.Name, c.Short)
-	if c.Help != "" {
-		fmt.Fprintf(os.Stdout, "  %s\n", c.Help)
-	}
-
-	for _, f := range c.Flags {
-		fmt.Fprintf(os.Stdout, "  -%s", f.Name)
-		if !f.Bool {
-			fmt.Fprintf(os.Stdout, " string")
-		}
-		fmt.Fprintf(os.Stdout, "\n    \t%s", f.Usage)
-		if f.Default != "" && f.Default != "false" {
-			fmt.Fprintf(os.Stdout, " (default %s)", f.Default)
-		}
-		fmt.Fprintln(os.Stdout)
-	}
-}

cmd/internal/ctl/ctl_test.go

@@ -1,6 +1,7 @@
 package ctl
 
 import (
+	"encoding/json"
 	"flag"
 	"os"
 	"path/filepath"
@@ -138,6 +139,98 @@ func TestParseCommandReportsWhatFlagSaid(t *testing.T) {
 	}
 }
 
+// TestParseCommandSendsATextlessCommandAsText: whether a command without a human form
+// can answer in text depends on what it returns, which the client cannot know until it
+// asks. A refusal always has a human form, so the request goes out as text and the
+// daemon decides.
+func TestParseCommandSendsATextlessCommandAsText(t *testing.T) {
+	schema := []commands.CommandSchema{{Name: "print-tunnel"}}
+
+	req, printedHelp, err := parseCommand(schema, []string{"print-tunnel"})
+	require.NoError(t, err)
+	assert.False(t, printedHelp)
+	assert.Equal(t, commands.PrintTunnel, req.name)
+	assert.Nil(t, req.Params)
+	assert.Empty(t, req.Args)
+	assert.Equal(t, commands.FormatUnset, req.Format)
+	assert.Equal(t, "text", wireFormat(req.Format))
+}
+
+// TestParseCommandOffPresentationFlagIsNoPreference: the daemon treats -json=false as
+// no preference, so nebula ctl must too, or the same line renders differently on the
+// two surfaces. Only a true value asks for JSON, and -pretty implies it.
+func TestParseCommandOffPresentationFlagIsNoPreference(t *testing.T) {
+	schema := []commands.CommandSchema{{
+		Name: "device-info",
+		Flags: []commands.FlagSchema{
+			{Name: commands.FlagJSON, Bool: true, Default: "false"},
+			{Name: commands.FlagPretty, Bool: true, Default: "false"},
+		},
+	}}
+
+	for _, tc := range []struct {
+		argv []string
+		want commands.Format
+		wire string
+	}{
+		{[]string{"device-info"}, commands.FormatUnset, "text"},
+		{[]string{"device-info", "-json=false"}, commands.FormatUnset, "text"},
+		{[]string{"device-info", "-pretty=false"}, commands.FormatUnset, "text"},
+		{[]string{"device-info", "-json=false", "-pretty=false"}, commands.FormatUnset, "text"},
+		{[]string{"device-info", "-json"}, commands.FormatJSON, "json"},
+		{[]string{"device-info", "-json=true"}, commands.FormatJSON, "json"},
+		{[]string{"device-info", "-pretty"}, commands.FormatPrettyJSON, "json"},
+		{[]string{"device-info", "-pretty", "-json=false"}, commands.FormatPrettyJSON, "json"},
+	} {
+		t.Run(strings.Join(tc.argv[1:], " "), func(t *testing.T) {
+			req, printedHelp, err := parseCommand(schema, tc.argv)
+			require.NoError(t, err)
+			assert.False(t, printedHelp)
+			assert.Equal(t, tc.want, req.Format)
+			assert.Equal(t, tc.wire, wireFormat(req.Format))
+			assert.Nil(t, req.Params, "presentation flags are never sent as parameters")
+		})
+	}
+}
+
+// TestParseCommandReadsTheLineAsTheDaemonDoes: nebula ctl parses the line with the same
+// rules as the daemon, so an operand after "--" reaches the command on both and a
+// negative number before it is the flag it would be at an ssh prompt.
+func TestParseCommandReadsTheLineAsTheDaemonDoes(t *testing.T) {
+	schema := []commands.CommandSchema{{Name: "set"}}
+
+	req, printedHelp, err := parseCommand(schema, []string{"set", "--", "-1"})
+	require.NoError(t, err)
+	assert.False(t, printedHelp)
+	assert.Equal(t, commands.Name("set"), req.name)
+	assert.Nil(t, req.Params)
+	assert.Equal(t, []string{"-1"}, req.Args)
+
+	_, _, err = parseCommand(schema, []string{"set", "-1"})
+	require.ErrorContains(t, err, "flag provided but not defined: -1")
+}
+
+// TestOutputRendersForTheFormatAsked: what the daemon answered is read by the format the
+// line asked for, never by sniffing the bytes. A text answer is the command's rendering
+// carried as a JSON string and comes back unquoted; a JSON answer whose value is a string
+// stays quoted, since to the script reading it that string is the JSON.
+func TestOutputRendersForTheFormatAsked(t *testing.T) {
+	text := json.RawMessage(`"Log level is: info\n"`)
+	assert.Equal(t, "Log level is: info\n", output(text, commands.FormatUnset))
+	assert.Equal(t, "Log level is: info\n", output(text, commands.FormatText))
+
+	str := json.RawMessage(`"some string"`)
+	assert.Equal(t, `"some string"`, output(str, commands.FormatJSON))
+	assert.Equal(t, `"some string"`, output(str, commands.FormatPrettyJSON))
+
+	obj := json.RawMessage(`{"a":1}`)
+	assert.Equal(t, `{"a":1}`, output(obj, commands.FormatJSON))
+	assert.Equal(t, "{\n    \"a\": 1\n}", output(obj, commands.FormatPrettyJSON))
+
+	assert.Empty(t, output(nil, commands.FormatJSON))
+	assert.Empty(t, output(nil, commands.FormatUnset))
+}
+
 // TestSplitLineLiftsOwnFlagsFromAnywhere: -config and -socket are the client's wherever
 // they appear, before the command or after it, in either spelling, so the two orders
 // mean the same line. Everything else after the command word stays in order for the
@@ -145,8 +238,8 @@ func TestParseCommandReportsWhatFlagSaid(t *testing.T) {
 func TestSplitLineLiftsOwnFlagsFromAnywhere(t *testing.T) {
 	newFS := func() *flag.FlagSet {
 		fs := flag.NewFlagSet("nebula ctl", flag.ContinueOnError)
-		fs.String("config", "", "")
-		fs.String("socket", "", "")
+		fs.String(commands.FlagConfig, "", "")
+		fs.String(commands.FlagSocket, "", "")
 		return fs
 	}
 

commands.go

@@ -1,48 +1,33 @@
 package nebula
 
 import (
-	"bytes"
 	"encoding/json"
 	"flag"
 	"fmt"
+	"io"
 	"log/slog"
 	"maps"
 	"net/netip"
 	"sort"
 	"strings"
 
+	"github.com/slackhq/nebula/cert"
 	"github.com/slackhq/nebula/commands"
 	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/header"
 	"github.com/slackhq/nebula/logging"
 )
 
 // The commands nebula serves. attachCommands builds the one registry every surface
-// reads. See the commands package for what a Command is.
+// reads; the ssh debug server is a consumer of it, not its owner, which is why this
+// is not ssh.go. See the commands package for what a Command is.
 
 type listHostMapFlags struct {
-	Json    bool
-	Pretty  bool
 	ByIndex bool
 }
 
-// listHostMapFlagSet is the flag set list-hostmap and list-pending-hostmap share.
-func listHostMapFlagSet() (*flag.FlagSet, any) {
-	fl := flag.NewFlagSet("", flag.ContinueOnError)
-	s := listHostMapFlags{}
-	fl.BoolVar(&s.Json, "json", false, "outputs as json with more information")
-	fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json")
-	fl.BoolVar(&s.ByIndex, "by-index", false, "gets all hosts in the hostmap from the index table")
-	return fl, &s
-}
-
 type printCertFlags struct {
-	Json   bool
-	Pretty bool
-	Raw    bool
-}
-
-type printTunnelFlags struct {
-	Pretty bool
+	Raw bool
 }
 
 type changeRemoteFlags struct {
@@ -57,195 +42,205 @@ type createTunnelFlags struct {
 	Address string
 }
 
-type deviceInfoFlags struct {
-	Json   bool
-	Pretty bool
+// lighthouseInfo is one entry of list-lighthouse-addrmap.
+type lighthouseInfo struct {
+	VpnAddr string    `json:"vpnAddr"`
+	Addrs   *CacheMap `json:"addrs"`
+}
+
+// deviceInfo is the result of device-info.
+type deviceInfo struct {
+	Name string         `json:"name"`
+	Cidr []netip.Prefix `json:"cidr"`
+}
+
+// certPEM is the result of print-cert -raw. The PEM form is a different thing to emit,
+// not a different formatting of the certificate, so it is its own shape.
+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.
+type relayFor struct {
+	Error          string `json:",omitempty"`
+	Type           string
+	State          string
+	PeerAddr       netip.Addr
+	LocalIndex     uint32
+	RemoteIndex    uint32
+	RelayedThrough []netip.Addr
+}
+
+type relayOutput struct {
+	NebulaAddr    netip.Addr
+	RelayForAddrs []relayFor
+}
+
+type relaysOutput struct {
+	Relays []*relayOutput
+}
+
+type versionInfo struct {
+	Version string `json:"version"`
+}
+
+type logLevelInfo struct {
+	Level string `json:"level"`
+}
+
+type logFormatInfo struct {
+	Format string `json:"format"`
+}
+
+// defineListHostMapFlags declares the flags list-hostmap and list-pending-hostmap share.
+func defineListHostMapFlags(fl *flag.FlagSet, s *listHostMapFlags) {
+	fl.BoolVar(&s.ByIndex, "by-index", false, "gets all hosts in the hostmap from the index table")
 }
 
 func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Interface) {
-	reg.Register(&commands.Command{
+	reg.Register(commands.Flagged(&commands.Command{
 		Name:             commands.ListHostmap,
 		ShortDescription: "List all known previously connected hosts",
-		Flags:            listHostMapFlagSet,
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return listHostMapCommand(f.hostMap, fs, w)
-		},
-	})
+		Text:             commands.Typed(textListHostMap),
+	}, defineListHostMapFlags, func(s *listHostMapFlags, a []string) (commands.Result, error) {
+		return runListHostMap(f.hostMap, s)
+	}))
 
-	reg.Register(&commands.Command{
+	reg.Register(commands.Flagged(&commands.Command{
 		Name:             commands.ListPendingHostmap,
 		ShortDescription: "List all handshaking hosts",
-		Flags:            listHostMapFlagSet,
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return listHostMapCommand(f.handshakeManager, fs, w)
-		},
-	})
+		Text:             commands.Typed(textListHostMap),
+	}, defineListHostMapFlags, func(s *listHostMapFlags, a []string) (commands.Result, error) {
+		return runListHostMap(f.handshakeManager, s)
+	}))
 
 	reg.Register(&commands.Command{
 		Name:             commands.ListLighthouseAddrmap,
 		ShortDescription: "List all lighthouse map entries",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := listHostMapFlags{}
-			fl.BoolVar(&s.Json, "json", false, "outputs as json with more information")
-			fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return listLighthouseAddrmapCommand(f.lightHouse, fs, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return runListLighthouseMap(f.lightHouse)
 		},
+		Text: commands.Typed(textListLighthouseMap),
 	})
 
 	reg.Register(&commands.Command{
 		Name:             commands.Reload,
 		ShortDescription: "Reloads configuration from disk, same as sending HUP to the process",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return reloadCommand(l, c, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return runReload(l, c)
 		},
 	})
 
 	reg.Register(&commands.Command{
 		Name:             commands.LogLevel,
 		ShortDescription: "Gets or sets the current log level",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return logLevelCommand(l, fs, a, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return runLogLevel(l, a)
 		},
+		Text: commands.Typed(textLogLevel),
 	})
 
 	reg.Register(&commands.Command{
 		Name:             commands.LogFormat,
 		ShortDescription: "Gets or sets the current log format",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return logFormatCommand(l, fs, a, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return runLogFormat(l, a)
 		},
+		Text: commands.Typed(textLogFormat),
 	})
 
 	reg.Register(&commands.Command{
 		Name:             commands.Version,
 		ShortDescription: "Prints the currently running version of nebula",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return versionCommand(f, fs, a, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value(versionInfo{Version: f.version}), nil
 		},
+		Text: commands.Typed(textVersion),
 	})
 
 	reg.Register(&commands.Command{
 		Name:             commands.DeviceInfo,
 		ShortDescription: "Prints information about the network device.",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := deviceInfoFlags{}
-			fl.BoolVar(&s.Json, "json", false, "outputs as json with more information")
-			fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return deviceInfoCommand(f, fs, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value(runDeviceInfo(f)), nil
 		},
+		Text: commands.Typed(textDeviceInfo),
 	})
 
-	reg.Register(&commands.Command{
+	reg.Register(commands.Flagged(&commands.Command{
 		Name:             commands.PrintCert,
 		ShortDescription: "Prints the current certificate being used or the certificate for the provided vpn addr",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := printCertFlags{}
-			fl.BoolVar(&s.Json, "json", false, "outputs as json")
-			fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json")
-			fl.BoolVar(&s.Raw, "raw", false, "raw prints the PEM encoded certificate, not compatible with -json or -pretty")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return printCertCommand(f, fs, a, w)
-		},
-	})
+		Text:             textPrintCert,
+	}, func(fl *flag.FlagSet, s *printCertFlags) {
+		fl.BoolVar(&s.Raw, "raw", false, "prints the PEM encoded certificate; with -json, an object holding it")
+	}, func(s *printCertFlags, a []string) (commands.Result, error) {
+		return runPrintCert(f, s, a)
+	}))
 
 	reg.Register(&commands.Command{
 		Name:             commands.PrintTunnel,
 		ShortDescription: "Prints json details about a tunnel for the provided vpn addr",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := printTunnelFlags{}
-			fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return printTunnelCommand(f, fs, a, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return runPrintTunnel(f, a)
 		},
+		// No Text on purpose: nothing human readable exists yet, and shipping an interim
+		// form would invite scripts against it. ssh defaults these to JSON; ctl asks.
+		JSONOnly: true,
 	})
 
 	reg.Register(&commands.Command{
 		Name:             commands.PrintRelays,
 		ShortDescription: "Prints json details about all relay info",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := printTunnelFlags{}
-			fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return printRelaysCommand(f, fs, a, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value(runPrintRelays(f)), nil
 		},
+		// No Text on purpose; see print-tunnel.
+		JSONOnly: true,
 	})
 
-	reg.Register(&commands.Command{
+	reg.Register(commands.Flagged(&commands.Command{
 		Name:             commands.ChangeRemote,
 		ShortDescription: "Changes the remote address used in the tunnel for the provided vpn addr",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := changeRemoteFlags{}
-			fl.StringVar(&s.Address, "address", "", "The new remote address, ip:port")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return changeRemoteCommand(f, fs, a, w)
-		},
-	})
+	}, func(fl *flag.FlagSet, s *changeRemoteFlags) {
+		fl.StringVar(&s.Address, "address", "", "The new remote address, ip:port")
+	}, func(s *changeRemoteFlags, a []string) (commands.Result, error) {
+		return runChangeRemote(f, s, a)
+	}))
 
-	reg.Register(&commands.Command{
+	reg.Register(commands.Flagged(&commands.Command{
 		Name:             commands.CloseTunnel,
 		ShortDescription: "Closes a tunnel for the provided vpn addr",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := closeTunnelFlags{}
-			fl.BoolVar(&s.LocalOnly, "local-only", false, "Disables notifying the remote that the tunnel is shutting down")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return closeTunnelCommand(f, fs, a, w)
-		},
-	})
+	}, func(fl *flag.FlagSet, s *closeTunnelFlags) {
+		fl.BoolVar(&s.LocalOnly, "local-only", false, "Disables notifying the remote that the tunnel is shutting down")
+	}, func(s *closeTunnelFlags, a []string) (commands.Result, error) {
+		return runCloseTunnel(f, s, a)
+	}))
 
-	reg.Register(&commands.Command{
+	reg.Register(commands.Flagged(&commands.Command{
 		Name:             commands.CreateTunnel,
 		ShortDescription: "Creates a tunnel for the provided vpn address",
 		Help:             "The lighthouses will be queried for real addresses but you can provide one as well.",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := createTunnelFlags{}
-			fl.StringVar(&s.Address, "address", "", "Optionally provide a real remote address, ip:port ")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return createTunnelCommand(f, fs, a, w)
-		},
-	})
+	}, func(fl *flag.FlagSet, s *createTunnelFlags) {
+		fl.StringVar(&s.Address, "address", "", "Optionally provide a real remote address, ip:port ")
+	}, func(s *createTunnelFlags, a []string) (commands.Result, error) {
+		return runCreateTunnel(f, s, a)
+	}))
 
 	reg.Register(&commands.Command{
 		Name:             commands.QueryLighthouse,
 		ShortDescription: "Query the lighthouses for the provided vpn address",
 		Help:             "This command is asynchronous. Only currently known udp addresses will be printed.",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return queryLighthouseCommand(f, fs, a, w)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return runQueryLighthouse(f, a)
 		},
+		// No Text on purpose; see print-tunnel.
+		JSONOnly: true,
 	})
 }
 
-func listHostMapCommand(hl controlHostLister, a any, w commands.StringWriter) error {
-	fs, ok := a.(*listHostMapFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *listHostMapFlags but was %T", a)
-	}
-
+// runListHostMap computes the hostmap listing. Presentation is Render's job.
+func runListHostMap(hl controlHostLister, fs *listHostMapFlags) (commands.Result, error) {
 	var hm []ControlHostInfo
 	if fs.ByIndex {
 		hm = listHostMapIndexes(hl)
@@ -257,39 +252,21 @@ func listHostMapCommand(hl controlHostLister, a any, w commands.StringWriter) er
 		return hm[i].VpnAddrs[0].Compare(hm[j].VpnAddrs[0]) < 0
 	})
 
-	if fs.Json || fs.Pretty {
-		js := json.NewEncoder(w.GetWriter())
-		if fs.Pretty {
-			js.SetIndent("", "    ")
-		}
+	return commands.Value(hm), nil
+}
 
-		if err := js.Encode(hm); err != nil {
+// textListHostMap is the human rendering, shared by every surface.
+func textListHostMap(hm []ControlHostInfo, w io.Writer) error {
+	for _, h := range hm {
+		if _, err := fmt.Fprintf(w, "%s: %s\n", h.VpnAddrs, h.RemoteAddrs); err != nil {
 			return err
 		}
-
-	} else {
-		for _, v := range hm {
-			err := w.WriteLine(fmt.Sprintf("%s: %s", v.VpnAddrs, v.RemoteAddrs))
-			if err != nil {
-				return err
-			}
-		}
 	}
 
 	return nil
 }
 
-func listLighthouseAddrmapCommand(lightHouse *LightHouse, a any, w commands.StringWriter) error {
-	fs, ok := a.(*listHostMapFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *listHostMapFlags but was %T", a)
-	}
-
-	type lighthouseInfo struct {
-		VpnAddr string    `json:"vpnAddr"`
-		Addrs   *CacheMap `json:"addrs"`
-	}
-
+func runListLighthouseMap(lightHouse *LightHouse) (commands.Result, error) {
 	lightHouse.RLock()
 	addrMap := make([]lighthouseInfo, len(lightHouse.addrMap))
 	x := 0
@@ -306,56 +283,36 @@ func listLighthouseAddrmapCommand(lightHouse *LightHouse, a any, w commands.Stri
 		return strings.Compare(addrMap[i].VpnAddr, addrMap[j].VpnAddr) < 0
 	})
 
-	if fs.Json || fs.Pretty {
-		js := json.NewEncoder(w.GetWriter())
-		if fs.Pretty {
-			js.SetIndent("", "    ")
-		}
+	return commands.Value(addrMap), nil
+}
 
-		if err := js.Encode(addrMap); err != nil {
+func textListLighthouseMap(addrMap []lighthouseInfo, w io.Writer) error {
+	for _, e := range addrMap {
+		b, err := json.Marshal(e.Addrs)
+		if err != nil {
 			return err
 		}
-
-	} else {
-		for _, v := range addrMap {
-			b, err := json.Marshal(v.Addrs)
-			if err != nil {
-				return err
-			}
-			err = w.WriteLine(fmt.Sprintf("%s: %s", v.VpnAddr, string(b)))
-			if err != nil {
-				return err
-			}
+		if _, err := fmt.Fprintf(w, "%s: %s\n", e.VpnAddr, string(b)); err != nil {
+			return err
 		}
 	}
 
 	return nil
 }
 
-// parseVpnAddr parses the vpn address a command takes as its first operand, failing to w
-// when there is none or it does not parse. noun is how the command's messages name it,
-// "vpn address" or "vpn addr", which the output golden pins per command.
-func parseVpnAddr(a []string, noun string, w commands.StringWriter) (netip.Addr, error) {
+func textVersion(vi versionInfo, w io.Writer) error {
+	_, err := fmt.Fprintf(w, "%s\n", vi.Version)
+	return err
+}
+
+func runQueryLighthouse(ifce *Interface, a []string) (commands.Result, error) {
 	if len(a) == 0 {
-		return netip.Addr{}, commands.Fail(w, "No vpn address was provided")
+		return commands.Failed("No vpn address was provided"), nil
 	}
 
 	vpnAddr, err := netip.ParseAddr(a[0])
-	if err != nil {
-		return netip.Addr{}, commands.Fail(w, fmt.Sprintf("The provided %s could not be parsed: %s", noun, a[0]))
-	}
-
-	return vpnAddr, nil
-}
-
-func versionCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	return w.WriteLine(ifce.version)
-}
-
-func queryLighthouseCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	vpnAddr, err := parseVpnAddr(a, "vpn address", w)
-	if err != nil {
-		return err
+	if err != nil || !vpnAddr.IsValid() {
+		return commands.Failed("The provided vpn address could not be parsed: %s", a[0]), nil
 	}
 
 	var cm *CacheMap
@@ -363,59 +320,65 @@ func queryLighthouseCommand(ifce *Interface, fs any, a []string, w commands.Stri
 	if rl != nil {
 		cm = rl.CopyCache()
 	}
-	return json.NewEncoder(w.GetWriter()).Encode(cm)
+	return commands.Value(cm), nil
 }
 
-func closeTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	flags, ok := fs.(*closeTunnelFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *closeTunnelFlags but was %T", fs)
+func runCloseTunnel(ifce *Interface, flags *closeTunnelFlags, a []string) (commands.Result, error) {
+
+	if len(a) == 0 {
+		return commands.Failed("No vpn address was provided"), nil
 	}
 
-	vpnAddr, err := parseVpnAddr(a, "vpn address", w)
-	if err != nil {
-		return err
+	vpnAddr, err := netip.ParseAddr(a[0])
+	if err != nil || !vpnAddr.IsValid() {
+		return commands.Failed("The provided vpn address could not be parsed: %s", a[0]), nil
 	}
 
 	hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr)
 	if hostInfo == nil {
-		return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0]))
+		return commands.Failed("Could not find tunnel for vpn address: %v", a[0]), nil
 	}
 
 	if !flags.LocalOnly {
-		ifce.sendCloseTunnel(hostInfo)
+		ifce.send(
+			header.CloseTunnel,
+			0,
+			hostInfo.ConnectionState,
+			hostInfo,
+			[]byte{},
+			make([]byte, 12, 12),
+			make([]byte, mtu),
+		)
 	}
 
 	ifce.closeTunnel(hostInfo)
-	return w.WriteLine("Closed")
+	return commands.Did("Closed"), nil
 }
 
-func createTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	flags, ok := fs.(*createTunnelFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *createTunnelFlags but was %T", fs)
+func runCreateTunnel(ifce *Interface, flags *createTunnelFlags, a []string) (commands.Result, error) {
+
+	if len(a) == 0 {
+		return commands.Failed("No vpn address was provided"), nil
 	}
 
-	vpnAddr, err := parseVpnAddr(a, "vpn address", w)
-	if err != nil {
-		return err
+	vpnAddr, err := netip.ParseAddr(a[0])
+	if err != nil || !vpnAddr.IsValid() {
+		return commands.Failed("The provided vpn address could not be parsed: %s", a[0]), nil
 	}
 
-	hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr)
-	if hostInfo != nil {
-		return commands.Fail(w, "Tunnel already exists")
+	if hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr); hostInfo != nil {
+		return commands.Failed("Tunnel already exists"), nil
 	}
 
-	hostInfo = ifce.handshakeManager.QueryVpnAddr(vpnAddr)
-	if hostInfo != nil {
-		return commands.Fail(w, "Tunnel already handshaking")
+	if hostInfo := ifce.handshakeManager.QueryVpnAddr(vpnAddr); hostInfo != nil {
+		return commands.Failed("Tunnel already handshaking"), nil
 	}
 
 	var addr netip.AddrPort
 	if flags.Address != "" {
 		addr, err = netip.ParseAddrPort(flags.Address)
 		if err != nil {
-			return commands.Fail(w, "Address could not be parsed")
+			return commands.Failed("Address could not be parsed"), nil
 		}
 	}
 
@@ -428,176 +391,148 @@ func createTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringW
 		}
 	})
 
-	return w.WriteLine("Created")
+	return commands.Did("Created"), nil
 }
 
-func changeRemoteCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	flags, ok := fs.(*changeRemoteFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *changeRemoteFlags but was %T", fs)
-	}
+func runChangeRemote(ifce *Interface, flags *changeRemoteFlags, a []string) (commands.Result, error) {
 
-	vpnAddr, err := parseVpnAddr(a, "vpn address", w)
-	if err != nil {
-		return err
+	if len(a) == 0 {
+		return commands.Failed("No vpn address was provided"), nil
 	}
 
 	if flags.Address == "" {
-		return commands.Fail(w, "No address was provided")
+		return commands.Failed("No address was provided"), nil
 	}
 
 	addr, err := netip.ParseAddrPort(flags.Address)
 	if err != nil {
-		return commands.Fail(w, "Address could not be parsed")
+		return commands.Failed("Address could not be parsed"), nil
+	}
+
+	vpnAddr, err := netip.ParseAddr(a[0])
+	if err != nil || !vpnAddr.IsValid() {
+		return commands.Failed("The provided vpn address could not be parsed: %s", a[0]), nil
 	}
 
 	hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr)
 	if hostInfo == nil {
-		return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0]))
+		return commands.Failed("Could not find tunnel for vpn address: %v", a[0]), nil
 	}
 
 	hostInfo.SetRemote(addr)
-	return w.WriteLine("Changed")
+	return commands.Did("Changed"), nil
 }
 
-func logLevelCommand(l *slog.Logger, fs any, a []string, w commands.StringWriter) error {
+func runLogLevel(l *slog.Logger, a []string) (commands.Result, error) {
 	ctrl, ok := l.Handler().(interface {
 		GetLevel() slog.Level
 		SetLevel(slog.Level)
 	})
 	if !ok {
-		return commands.Fail(w, "Log level is not reconfigurable on this logger")
+		return commands.Failed("Log level is not reconfigurable on this logger"), nil
 	}
 
-	if len(a) == 0 {
-		return w.WriteLine(fmt.Sprintf("Log level is: %s", logging.LevelName(ctrl.GetLevel())))
-	}
+	if len(a) > 0 {
+		level, err := logging.ParseLevel(strings.ToLower(a[0]))
+		if err != nil {
+			return commands.Failed("Unknown log level %s. Possible log levels: trace, debug, info, warn, error", a), nil
+		}
 
-	level, err := logging.ParseLevel(strings.ToLower(a[0]))
-	if err != nil {
-		return commands.Fail(w, fmt.Sprintf("Unknown log level %s. Possible log levels: trace, debug, info, warn, error", a))
+		ctrl.SetLevel(level)
 	}
 
-	ctrl.SetLevel(level)
-	return w.WriteLine(fmt.Sprintf("Log level is: %s", logging.LevelName(ctrl.GetLevel())))
+	return commands.Value(logLevelInfo{Level: logging.LevelName(ctrl.GetLevel())}), nil
 }
 
-func logFormatCommand(l *slog.Logger, fs any, a []string, w commands.StringWriter) error {
+func textLogLevel(li logLevelInfo, w io.Writer) error {
+	_, err := fmt.Fprintf(w, "Log level is: %s\n", li.Level)
+	return err
+}
+
+func runLogFormat(l *slog.Logger, a []string) (commands.Result, error) {
 	ctrl, ok := l.Handler().(interface {
 		GetFormat() string
 		SetFormat(string) error
 	})
 	if !ok {
-		return commands.Fail(w, "Log format is not reconfigurable on this logger")
+		return commands.Failed("Log format is not reconfigurable on this logger"), nil
 	}
 
-	if len(a) == 0 {
-		return w.WriteLine(fmt.Sprintf("Log format is: %s", ctrl.GetFormat()))
+	if len(a) > 0 {
+		if err := ctrl.SetFormat(strings.ToLower(a[0])); err != nil {
+			return commands.Failed("Unable to set log format: %s", err), nil
+		}
 	}
 
-	if err := ctrl.SetFormat(strings.ToLower(a[0])); err != nil {
-		return commands.Fail(w, fmt.Sprintf("Unable to set log format: %s", err))
-	}
-	return w.WriteLine(fmt.Sprintf("Log format is: %s", ctrl.GetFormat()))
+	return commands.Value(logFormatInfo{Format: ctrl.GetFormat()}), nil
 }
 
-func printCertCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	args, ok := fs.(*printCertFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *printCertFlags but was %T", fs)
-	}
+func textLogFormat(fi logFormatInfo, w io.Writer) error {
+	_, err := fmt.Fprintf(w, "Log format is: %s\n", fi.Format)
+	return err
+}
+
+func runPrintCert(ifce *Interface, args *printCertFlags, a []string) (commands.Result, error) {
 
-	cert := ifce.pki.getCertState().GetDefaultCertificate()
+	c := ifce.pki.getCertState().GetDefaultCertificate()
 	if len(a) > 0 {
-		vpnAddr, err := parseVpnAddr(a, "vpn addr", w)
-		if err != nil {
-			return err
+		vpnAddr, err := netip.ParseAddr(a[0])
+		if err != nil || !vpnAddr.IsValid() {
+			return commands.Failed("The provided vpn addr could not be parsed: %s", a[0]), nil
 		}
 
 		hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr)
 		if hostInfo == nil {
-			return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0]))
-		}
-
-		cert = hostInfo.GetCert().Certificate
-	}
-
-	if args.Json || args.Pretty {
-		b, err := cert.MarshalJSON()
-		if err != nil {
-			return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err))
+			return commands.Failed("Could not find tunnel for vpn addr: %v", a[0]), nil
 		}
 
-		if args.Pretty {
-			buf := new(bytes.Buffer)
-			if err := json.Indent(buf, b, "", "    "); err != nil {
-				return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err))
-			}
-			b = buf.Bytes()
-		}
-
-		return w.WriteBytes(b)
+		c = hostInfo.GetCert().Certificate
 	}
 
 	if args.Raw {
-		b, err := cert.MarshalPEM()
+		b, err := c.MarshalPEM()
 		if err != nil {
-			return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err))
+			return commands.Result{}, err
 		}
-
-		return w.WriteBytes(b)
+		return commands.Value(certPEM{PEM: string(b)}), nil
 	}
 
-	return w.WriteLine(cert.String())
+	return commands.Value(c), nil
 }
 
-func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	args, ok := fs.(*printTunnelFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *printTunnelFlags but was %T", fs)
-	}
-
-	ifce.hostMap.RLock()
-	relays := maps.Clone(ifce.hostMap.Relays)
-	ifce.hostMap.RUnlock()
-
-	type RelayFor struct {
-		Error          string `json:",omitempty"`
-		Type           string
-		State          string
-		PeerAddr       netip.Addr
-		LocalIndex     uint32
-		RemoteIndex    uint32
-		RelayedThrough []netip.Addr
-	}
-
-	type RelayOutput struct {
-		NebulaAddr    netip.Addr
-		RelayForAddrs []RelayFor
-	}
-
-	type CmdOutput struct {
-		Relays []*RelayOutput
+// textPrintCert takes the value untyped: print-cert returns a certificate or, with -raw,
+// its PEM, and those are two things to emit rather than two renderings of one.
+func textPrintCert(v any, w io.Writer) error {
+	switch c := v.(type) {
+	case certPEM:
+		_, err := io.WriteString(w, c.PEM)
+		return err
+	case cert.Certificate:
+		_, err := io.WriteString(w, c.String()+"\n")
+		return err
+	default:
+		return fmt.Errorf("expected a certificate, got %T", v)
 	}
+}
 
-	co := CmdOutput{}
-
-	enc := json.NewEncoder(w.GetWriter())
+func runPrintRelays(ifce *Interface) any {
+	relays := map[uint32]*HostInfo{}
+	ifce.hostMap.Lock()
+	maps.Copy(relays, ifce.hostMap.Relays)
+	ifce.hostMap.Unlock()
 
-	if args.Pretty {
-		enc.SetIndent("", "    ")
-	}
+	co := relaysOutput{}
 
 	for k, v := range relays {
-		ro := RelayOutput{NebulaAddr: v.vpnAddrs[0]}
+		ro := relayOutput{NebulaAddr: v.vpnAddrs[0]}
 		co.Relays = append(co.Relays, &ro)
 		relayHI := ifce.hostMap.QueryVpnAddr(v.vpnAddrs[0])
 		if relayHI == nil {
-			ro.RelayForAddrs = append(ro.RelayForAddrs, RelayFor{Error: "could not find hostinfo"})
+			ro.RelayForAddrs = append(ro.RelayForAddrs, relayFor{Error: "could not find hostinfo"})
 			continue
 		}
 		for _, vpnAddr := range relayHI.relayState.CopyRelayForIps() {
-			rf := RelayFor{}
+			rf := relayFor{}
 			r, ok := relayHI.relayState.GetRelayForByAddr(vpnAddr)
 			if ok {
 				t := ""
@@ -610,21 +545,21 @@ func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWr
 					t = "unknown"
 				}
 
-				s := ""
+				st := ""
 				switch r.State {
 				case Requested:
-					s = "requested"
+					st = "requested"
 				case Established:
-					s = "established"
+					st = "established"
 				default:
-					s = "unknown"
+					st = "unknown"
 				}
 
 				rf.LocalIndex = r.LocalIndex
 				rf.RemoteIndex = r.RemoteIndex
 				rf.PeerAddr = r.PeerAddr
 				rf.Type = t
-				rf.State = s
+				rf.State = st
 				if rf.LocalIndex != k {
 					rf.Error = fmt.Sprintf("hostmap LocalIndex '%v' does not match RelayState LocalIndex", k)
 				}
@@ -637,74 +572,50 @@ func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWr
 			ro.RelayForAddrs = append(ro.RelayForAddrs, rf)
 		}
 	}
-	return enc.Encode(co)
+
+	return co
 }
 
-func printTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error {
-	args, ok := fs.(*printTunnelFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected *printTunnelFlags but was %T", fs)
+func runPrintTunnel(ifce *Interface, a []string) (commands.Result, error) {
+	if len(a) == 0 {
+		return commands.Failed("No vpn address was provided"), nil
 	}
 
-	vpnAddr, err := parseVpnAddr(a, "vpn addr", w)
-	if err != nil {
-		return err
+	vpnAddr, err := netip.ParseAddr(a[0])
+	if err != nil || !vpnAddr.IsValid() {
+		return commands.Failed("The provided vpn addr could not be parsed: %s", a[0]), nil
 	}
 
 	hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr)
 	if hostInfo == nil {
-		return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0]))
-	}
-
-	enc := json.NewEncoder(w.GetWriter())
-	if args.Pretty {
-		enc.SetIndent("", "    ")
+		return commands.Failed("Could not find tunnel for vpn addr: %v", a[0]), nil
 	}
 
-	return enc.Encode(copyHostInfo(hostInfo, ifce.hostMap.GetPreferredRanges()))
+	return commands.Value(copyHostInfo(hostInfo, ifce.hostMap.GetPreferredRanges())), nil
 }
 
-func deviceInfoCommand(ifce *Interface, fs any, w commands.StringWriter) error {
-	flags, ok := fs.(*deviceInfoFlags)
-	if !ok {
-		return fmt.Errorf("internal error: expected flags to be deviceInfoFlags but was %+v", fs)
-	}
-
-	data := struct {
-		Name string         `json:"name"`
-		Cidr []netip.Prefix `json:"cidr"`
-	}{
+func runDeviceInfo(ifce *Interface) deviceInfo {
+	d := deviceInfo{
 		Name: ifce.inside.Name(),
 		Cidr: make([]netip.Prefix, len(ifce.inside.Networks())),
 	}
+	copy(d.Cidr, ifce.inside.Networks())
+	return d
+}
 
-	copy(data.Cidr, ifce.inside.Networks())
-
-	if flags.Json || flags.Pretty {
-		js := json.NewEncoder(w.GetWriter())
-		if flags.Pretty {
-			js.SetIndent("", "    ")
-		}
-
-		return js.Encode(data)
-	} else {
-		return w.WriteLine(fmt.Sprintf("name=%v cidr=%v", data.Name, data.Cidr))
-	}
+func textDeviceInfo(d deviceInfo, w io.Writer) error {
+	_, err := fmt.Fprintf(w, "name=%v cidr=%v\n", d.Name, d.Cidr)
+	return err
 }
 
-// reloadCommand reloads the config and writes the result to the client. It reloads even if the client has gone away
-func reloadCommand(l *slog.Logger, c *config.C, w commands.StringWriter) error {
-	writeErr := w.WriteLine("Reloading config")
+// runReload reloads the config and reports how it went, under the "Reloading config"
+// line the command has always printed first. A reload that failed is a refusal, exit 1
+// over ssh; one that merely needs a restart is not.
+func runReload(l *slog.Logger, c *config.C) (commands.Result, error) {
 	res := c.ReloadConfig()
 	res.Log(l)
-	if err := w.WriteLine(res.String()); writeErr == nil {
-		writeErr = err
-	}
-	if writeErr != nil {
-		return writeErr
-	}
 	if res.Err != nil || res.Failed {
-		return &commands.Failure{Msg: res.String()}
+		return commands.Failed("Reloading config\n%s", res.String()), nil
 	}
-	return nil
+	return commands.Did("Reloading config\n%s", res.String()), nil
 }

commands/command.go

@@ -1,23 +1,33 @@
 package commands
 
 import (
+	"errors"
 	"flag"
+	"fmt"
+	"io"
+	"strings"
 )
 
 // CommandFlags is called before help or command execution to parse command line flags.
 // It should return a flag.FlagSet instance and a pointer to the struct that will contain parsed flags
+//
+// Every call must build a fresh FlagSet and a fresh struct. Nothing may be shared between
+// calls: a command runs on more than one surface and on more than one goroutine.
 type CommandFlags func() (*flag.FlagSet, any)
 
-// CommandCallback is called when your command should execute.
-// fs will be a pointer to the struct provided by Command.Flags callback, if there was one. -h and -help are reserved
-// and handled automatically for you.
-// a will be any unconsumed arguments, if no Command.Flags was available this will be all the flags passed in.
-// w is the writer to use when sending messages back to the client.
-// An error returned by the callback is handed back to whoever dispatched the command, which reports a failure: exit
-// status 1 over ssh. The error is not shown to the user. For a failure the user caused or can act on, write the
-// message and return it as a *Failure, which Fail does in one step; the surface then reports the failure without
-// logging what the user was already told. Any other error is logged by the surface as well.
-type CommandCallback func(fs any, a []string, w StringWriter) error
+// RunFunc computes a command's Result.
+//
+// fs is the pointer returned by Command.Flags, if there was one; -h, -help, -json and
+// -pretty are reserved and handled before Run is called. a is the operands: whatever
+// was not consumed as a flag. The Result is rendered by whichever surface asked: Value
+// for a command that reports, Did or Failed for one that acts, Nothing for one with
+// nothing to say. An error is for a failure of the command itself; a bad argument is
+// Failed, so the caller hears why in its own format.
+type RunFunc func(fs any, a []string) (Result, error)
+
+// TextFunc renders a Result's Value for a person. See Typed for declaring one against
+// the type the command actually returns.
+type TextFunc func(v any, w io.Writer) error
 
 type Command struct {
 	Name             Name
@@ -26,13 +36,36 @@ type Command struct {
 
 	ForProfiling bool
 
-	Flags    CommandFlags
-	Callback CommandCallback
+	Flags CommandFlags
+
+	// Run produces the command's Result, leaving presentation to Render.
+	Run RunFunc
+
+	// Text renders a Result's Value for a person. Both surfaces call it, so a command
+	// reads the same over ssh as it does through the control socket. It never sees an
+	// outcome: Render prints those itself, the same way for every command. Leaving it
+	// nil declares that no human rendering of the Value exists yet, which JSONOnly says
+	// up front; Render then refuses text rather than inventing an interim form for
+	// people to script against.
+	Text TextFunc
+
+	// JSONOnly declares that the command's Value has no human rendering yet, so a surface
+	// whose default is text asks for JSON instead, and the schema says so to a client. It
+	// is a statement that no such form exists, not a fallback. A refusal is still
+	// rendered as a sentence, since that always has a human form.
+	JSONOnly bool
+
+	// AfterReply runs once the surface has delivered Run's result, and only then. It is
+	// for a command whose action would cut off its own reply: reload restarts the ssh
+	// server, which ends the session that asked, so the sentence Run returns has to be
+	// on the wire before the reload begins. Run promises; AfterReply acts.
+	AfterReply func()
 }
 
-// Failure is the error a command returns for a failure it has already explained to the user:
-// the message it wrote is the whole story. A surface reports it as a failure and does not
-// log it.
+// Failure is the error a surface gets for a refusal the person has already been shown:
+// a Run that returned Failed, a command that is not there, a line that would not parse.
+// The message it rendered is the whole story, so the surface reports the failure, exit
+// status 1 over ssh, and does not log it.
 type Failure struct {
 	Msg string
 }
@@ -41,48 +74,212 @@ func (f *Failure) Error() string {
 	return f.Msg
 }
 
-// Fail writes msg to w and returns it as a *Failure, so a command reports a failure to the
-// user and to its surface in one line. If the write itself fails, that error is returned
-// instead: the user was not told anything.
-func Fail(w StringWriter, msg string) error {
-	if err := w.WriteLine(msg); err != nil {
+// commandFlags returns the FlagSet a command is driven through, on any surface: the
+// command's own flags, if it declares any, with the two presentation flags added. own
+// reports whether the command declared any of its own, which is what tells a caller
+// whether to say a parameter is unknown or that the command takes none at all.
+//
+// The presentation flags are defined here rather than by each command, because how a
+// result is displayed is not a property of the command that produced it. This is also
+// the one definition of them: the ssh line parses into it, nebula ctl parses its line
+// against the schema derived from it, and help renders from that schema. They are
+// command line flags only: a socket request chooses its rendering with the envelope's
+// format field, and Invoke refuses them as parameters.
+func commandFlags(c *Command) (fl *flag.FlagSet, fs any, own bool) {
+	if c.Flags != nil {
+		fl, fs = c.Flags()
+	}
+	own = fl != nil
+	if fl == nil {
+		fl = flag.NewFlagSet(string(c.Name), flag.ContinueOnError)
+	}
+
+	addPresentationFlags(fl)
+
+	// What Parse prints after a bad flag, and for -h: the command's usage as help renders
+	// it, so a line that went wrong is answered with the same text as `help <command>`.
+	fl.Usage = func() { _ = WriteCommandHelp(schemaOf(c, fl), fl.Output()) }
+
+	return fl, fs, own
+}
+
+// addPresentationFlags defines the two presentation flags on fl.
+func addPresentationFlags(fl *flag.FlagSet) {
+	fl.Bool(FlagJSON, false, "outputs as json")
+	fl.Bool(FlagPretty, false, "pretty prints json, assumes -json")
+}
+
+// Flagged fills in c's Flags and Run as a pair: define declares the flags on a fresh F,
+// and run is handed that F. The struct the flags fill is the struct Run reads by
+// construction, so no Run has to assert the type it was given or decide what to do
+// when the assertion fails.
+func Flagged[F any](c *Command, define func(*flag.FlagSet, *F), run func(*F, []string) (Result, error)) *Command {
+	c.Flags = func() (*flag.FlagSet, any) {
+		fl := flag.NewFlagSet("", flag.ContinueOnError)
+		s := new(F)
+		define(fl, s)
+		return fl, s
+	}
+	c.Run = func(fs any, a []string) (Result, error) {
+		s, ok := fs.(*F)
+		if !ok {
+			return Result{}, fmt.Errorf("%s was handed %T for its flags, not %T", c.Name, fs, s)
+		}
+		return run(s, a)
+	}
+	return c
+}
+
+// isPresentationFlag reports whether name is one of the two flags commandFlags adds.
+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.
+func (r *Registry) execLine(c *Command, args []string, w Output) error {
+	fl, fs, _ := commandFlags(c)
+
+	// Parse reports a bad flag to this writer, with the command's usage after it. -h,
+	// -help and --help take the same path with nothing wrong: the usage is the whole
+	// answer, and Parse says so with ErrHelp. A -h that is a flag's value or follows --
+	// is read as the operand it is, since ParseArgs is the one reading of the line.
+	fl.SetOutput(w.Err)
+	operands, err := ParseArgs(fl, args)
+	if err != nil {
+		if errors.Is(err, flag.ErrHelp) {
+			return nil
+		}
+		return err
+	}
+
+	format := presentationFormat(fl)
+
+	res, err := c.Run(fs, operands)
+	if err != nil {
 		return err
 	}
-	return &Failure{Msg: msg}
+
+	if res.Empty() {
+		runAfterReply(c)
+		return nil
+	}
+
+	// The line wins when it asked for a format. 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.
+	f := format
+	if f == FormatUnset {
+		f = r.defaultFormatFor(c, res)
+	}
+
+	if err := Render(c, res, f, w.Out); err != nil {
+		if errors.Is(err, ErrNoTextRendering) {
+			_, _ = fmt.Fprintln(w.Err, err.Error())
+		}
+		return err
+	}
+
+	runAfterReply(c)
+	if !res.OK {
+		return &Failure{Msg: res.Reason}
+	}
+	return nil
 }
 
-// execCommand 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,
-// becomes a Request, and Invoke runs it exactly as it runs the control socket's.
-func execCommand(c *Command, args []string, w StringWriter) error {
-	req := Request{Command: c, Args: args}
+// runAfterReply performs the command's deferred action, if it has one. It is called only
+// once the reply has been written, which is the whole point of AfterReply.
+func runAfterReply(c *Command) {
+	if c.AfterReply != nil {
+		c.AfterReply()
+	}
+}
 
-	if c.Flags != nil {
-		if fl, _ := c.Flags(); fl != nil {
-			// SetOutput() here in case fl.Parse dumps usage.
-			fl.SetOutput(w.Err())
-			if err := fl.Parse(args); err != nil {
-				// fl.Parse has dumped error information to the user via the w writer.
-				return err
+// ParseArgs parses args with fl and returns the operands.
+//
+// It differs from fl.Parse in two ways, and both surfaces parse through here so a line
+// means the same typed at an ssh prompt or to nebula ctl.
+//
+// Flags and operands may come in any order, which is the one way it differs from
+// fl.Parse. Parse stops at the first operand, so `print-tunnel 10.0.0.1 -json` would
+// reach the command with "-json" as a second operand and the flag silently dropped. Here
+// an operand ends nothing: parsing resumes after it, and only "--" ends the flags, with
+// everything after it an operand. What counts as a flag is Parse's rule: every argument
+// that starts with "-", a negative number included, so one that is meant as an operand
+// goes after "--".
+func ParseArgs(fl *flag.FlagSet, args []string) ([]string, error) {
+	operands := make([]string, 0, len(args))
+
+	for len(args) > 0 {
+		// Walk the flags the way Parse will, to the first argument it would not read as a
+		// flag: an operand or the terminator. Everything before it is Parse's.
+		end := len(args)
+		for i := 0; i < len(args); {
+			a := args[i]
+			if a == "--" {
+				if err := fl.Parse(args[:i]); err != nil {
+					return nil, err
+				}
+				return append(operands, args[i+1:]...), nil
+			}
+
+			if len(a) < 2 || a[0] != '-' {
+				end = i
+				break
+			}
+
+			f, hasValue := LookupArg(fl, a)
+			if f == nil {
+				// An undefined flag: leave it for Parse to report.
+				i++
+				continue
+			}
+
+			if hasValue || IsBoolFlag(f) {
+				i++
+			} else {
+				// The flag's value is the next argument, whatever it looks like.
+				i += 2
 			}
-			req = ParseLine(fl, fl.Args())
-			req.Command = c
 		}
+
+		if err := fl.Parse(args[:end]); err != nil {
+			return nil, err
+		}
+
+		if end == len(args) {
+			break
+		}
+
+		operands = append(operands, args[end])
+		args = args[end+1:]
 	}
 
-	return Invoke(req, w)
+	return operands, nil
 }
 
-// IsHelpRequest reports whether args ask for a command's help rather than running it.
-// Both surfaces use it, so what counts as asking for help cannot drift between them: the
-// ssh server answers with Registry.Help, and nebula ctl renders the same help locally
-// from the schema.
-func IsHelpRequest(args []string) bool {
-	for _, a := range args {
-		if a == "-h" || a == "-help" || a == "--help" {
-			return true
-		}
+// LookupArg reads arg the way Parse does: one or two leading dashes, the flag's name, and
+// an optional "=value". f is the flag of fl by that name, nil when fl has none, and
+// hasValue reports that the value came with the argument rather than in the one after it.
+// It is the one reading of an argument's shape: ParseArgs walks a line with it, and
+// nebula ctl lifts its own flags out of a line with it, so the two cannot disagree about
+// which word is a flag's value.
+func LookupArg(fl *flag.FlagSet, arg string) (f *flag.Flag, hasValue bool) {
+	name := strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-")
+	if eq := strings.IndexByte(name, '='); eq >= 0 {
+		name, hasValue = name[:eq], true
 	}
+	return fl.Lookup(name), hasValue
+}
 
-	return false
+// IsBoolFlag reports whether f is satisfied by its presence alone and never reads the
+// argument after it as its value. The flag package marks these with an unexported
+// interface, which is the only way to tell.
+func IsBoolFlag(f *flag.Flag) bool {
+	bf, ok := f.Value.(interface{ IsBoolFlag() bool })
+	return ok && bf.IsBoolFlag()
 }

commands/command_test.go

@@ -0,0 +1,101 @@
+package commands
+
+import (
+	"bytes"
+	"flag"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestParseArgsAllowsFlagsAfterOperands pins the one way ParseArgs departs from fl.Parse,
+// that a flag may follow an operand, and that it departs in no other: a negative number
+// is a flag, as Parse reads it, and goes after "--" when it is meant as an operand.
+func TestParseArgsAllowsFlagsAfterOperands(t *testing.T) {
+	cases := map[string]struct {
+		args     []string
+		wantOps  []string
+		wantN    string
+		wantV    bool
+		wantJSON bool
+		wantErr  string
+	}{
+		"flag after operand":  {args: []string{"x", "-json"}, wantOps: []string{"x"}, wantJSON: true},
+		"flags around":        {args: []string{"-v", "x", "-n", "1", "y"}, wantOps: []string{"x", "y"}, wantV: true, wantN: "1"},
+		"value after operand": {args: []string{"x", "-n", "-1"}, wantOps: []string{"x"}, wantN: "-1"},
+		"equals value":        {args: []string{"-n=-1", "x"}, wantOps: []string{"x"}, wantN: "-1"},
+		"terminator after":    {args: []string{"x", "--", "-json", "-1"}, wantOps: []string{"x", "-json", "-1"}},
+		"terminator":          {args: []string{"--", "-json"}, wantOps: []string{"-json"}},
+		"defined numeric":     {args: []string{"-2", "x"}, wantOps: []string{"x"}, wantV: true},
+		"negative number":     {args: []string{"-1"}, wantErr: "flag provided but not defined: -1"},
+		"bad flag after":      {args: []string{"x", "-nope"}, wantErr: "flag provided but not defined: -nope"},
+		"undefined flag":      {args: []string{"-nope", "x"}, wantErr: "flag provided but not defined: -nope"},
+		"missing value":       {args: []string{"-n"}, wantErr: "flag needs an argument: -n"},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			fl := flag.NewFlagSet("t", flag.ContinueOnError)
+			fl.SetOutput(&bytes.Buffer{})
+			n := fl.String("n", "", "")
+			v := fl.Bool("v", false, "")
+			two := fl.Bool("2", false, "a flag that happens to be a number")
+			asJSON := fl.Bool(FlagJSON, false, "")
+
+			ops, err := ParseArgs(fl, tc.args)
+			if tc.wantErr != "" {
+				require.ErrorContains(t, err, tc.wantErr)
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantOps, ops)
+			assert.Equal(t, tc.wantN, *n)
+			assert.Equal(t, tc.wantV, *v || *two)
+			assert.Equal(t, tc.wantJSON, *asJSON)
+		})
+	}
+}
+
+// TestDispatchFlagAfterOperand: the same through a registered command with no Flags of
+// its own, which only has the presentation flags to find.
+func TestDispatchFlagAfterOperand(t *testing.T) {
+	out := &bytes.Buffer{}
+	require.NoError(t, testRegistry().DispatchArgs([]string{"served", "x", "-json"}, MergedOutput(out)))
+	assert.Equal(t, "\"read x\"\n", out.String())
+}
+
+// TestLookupArg pins the reading of an argument's shape that ParseArgs and nebula ctl
+// share.
+func TestLookupArg(t *testing.T) {
+	fl := flag.NewFlagSet("t", flag.ContinueOnError)
+	fl.String("n", "", "")
+	fl.Bool("v", false, "")
+
+	for _, tc := range []struct {
+		arg      string
+		name     string
+		hasValue bool
+	}{
+		{"-n", "n", false},
+		{"--n", "n", false},
+		{"-n=1", "n", true},
+		{"--n=", "n", true},
+		{"-v", "v", false},
+		{"-nope", "", false},
+		{"-1", "", false},
+	} {
+		f, hasValue := LookupArg(fl, tc.arg)
+		if tc.name == "" {
+			assert.Nil(t, f, tc.arg)
+		} else {
+			require.NotNil(t, f, tc.arg)
+			assert.Equal(t, tc.name, f.Name, tc.arg)
+		}
+		assert.Equal(t, tc.hasValue, hasValue, tc.arg)
+	}
+
+	assert.True(t, IsBoolFlag(fl.Lookup("v")))
+	assert.False(t, IsBoolFlag(fl.Lookup("n")))
+}

commands/registry.go

@@ -2,9 +2,9 @@ package commands
 
 import (
 	"cmp"
-	"errors"
 	"flag"
 	"fmt"
+	"io"
 	"maps"
 	"slices"
 	"strings"
@@ -13,11 +13,11 @@ import (
 	"github.com/anmitsu/go-shlex"
 )
 
-// Registry holds a set of commands. New is the only way to make one.
+// Registry holds a set of commands.
 //
-// It is safe for concurrent use. Commands may be registered while a surface is serving
-// from the registry: the ssh server clones it per session, and a clone may overlap a
-// Register.
+// It is safe for concurrent use. Commands may be registered while surfaces are serving
+// from the registry: the ssh server clones it per session and the control socket filters
+// it per request, and either may overlap a Register.
 type Registry struct {
 	mu       sync.RWMutex
 	commands map[Name]*Command
@@ -30,6 +30,21 @@ type Registry struct {
 	// a command they can name. It is supplied by whoever built the filter, because why a
 	// surface declines a command is that surface's business, not the registry's.
 	reason string
+
+	// DefaultFormat decides how a result is rendered when the command line named no
+	// format. It sees the command and its Result, and applies to values and refusals
+	// alike: the registry does not special-case either. The renderer is shared between
+	// surfaces; what each asks for by default is not, so the surface that owns this
+	// registry sets it. Nil means text.
+	DefaultFormat func(c *Command, r Result) Format
+}
+
+// defaultFormatFor resolves the surface's default for c's Result.
+func (r *Registry) defaultFormatFor(c *Command, res Result) Format {
+	if r.DefaultFormat == nil {
+		return FormatText
+	}
+	return r.DefaultFormat(c, res)
 }
 
 // New returns an empty Registry with only `help` available.
@@ -43,15 +58,111 @@ func (r *Registry) registerHelp() {
 	r.Register(&Command{
 		Name:             Help,
 		ShortDescription: "prints available commands or help <command> for specific usage info",
-		Callback: func(a any, args []string, w StringWriter) error {
-			return r.help(args, w)
+		Run: func(a any, args []string) (Result, error) {
+			if len(args) == 0 {
+				return Value(r.Schema()), nil
+			}
+
+			if c := r.Get(Name(args[0])); c != nil {
+				return Value(r.schemaFor(c)), nil
+			}
+
+			if err := r.unavailable(Name(args[0])); err != nil {
+				return Failed("%s", err), nil
+			}
+
+			return Failed("Command not available %s", args[0]), nil
 		},
+		Text: renderHelp,
 	})
 }
 
-// Register adds a command that can be run by a user. A command registered under a name
-// already held replaces it; see Clone for what that means for Help.
+// renderHelp is help's human rendering: the command list, or one command's usage.
+func renderHelp(v any, w io.Writer) error {
+	switch h := v.(type) {
+	case []CommandSchema:
+		return writeCommandList(h, w)
+	case CommandSchema:
+		return WriteCommandHelp(h, w)
+	default:
+		return fmt.Errorf("expected a command schema, got %T", v)
+	}
+}
+
+func writeCommandList(cmds []CommandSchema, w io.Writer) error {
+	if _, err := io.WriteString(w, "Available commands:\n"); err != nil {
+		return err
+	}
+
+	// In the order the schema came, which is the display order; see Registry.All.
+	lines := make([]string, 0, len(cmds))
+	for _, c := range cmds {
+		lines = append(lines, fmt.Sprintf("%s - %s", c.Name, c.Short))
+	}
+
+	_, err := io.WriteString(w, strings.Join(lines, "\n")+"\n\n")
+	return err
+}
+
+// WriteCommandHelp renders one command's usage. It is the single rendering of a
+// CommandSchema for a person: the daemon uses it for `help <command>` on both surfaces,
+// and a client that already holds the schema uses it for `-h`, so the two cannot drift.
+func WriteCommandHelp(c CommandSchema, w io.Writer) error {
+	if _, err := fmt.Fprintf(w, "%s - %s\n", c.Name, c.Short); err != nil {
+		return err
+	}
+
+	if c.Help != "" {
+		if _, err := fmt.Fprintf(w, "  %s\n", c.Help); err != nil {
+			return err
+		}
+	}
+
+	// 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.
+	for _, f := range c.Flags {
+		kind := ""
+		if !f.Bool {
+			kind = " string"
+		}
+		def := ""
+		if f.Default != "" && f.Default != "false" {
+			def = fmt.Sprintf(" (default %s)", f.Default)
+		}
+		if _, err := fmt.Fprintf(w, "  -%s%s\n    \t%s%s\n", f.Name, kind, f.Usage, def); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// Register adds a command that can be run by a user.
+//
+// It panics if the command declares a reserved flag. That is a programming error, and
+// surfacing it here, at startup, is better than at the moment an operator first types
+// the command.
 func (r *Registry) Register(c *Command) {
+	if c.Run == nil {
+		panic(fmt.Sprintf("commands: %s has no Run", c.Name))
+	}
+
+	if c.Flags != nil {
+		if fl, _ := c.Flags(); fl != nil {
+			for _, name := range reservedFlags {
+				if fl.Lookup(name) != nil {
+					panic(fmt.Sprintf("commands: %s declares reserved flag -%s", c.Name, name))
+				}
+			}
+		}
+	}
+
+	r.insert(c)
+}
+
+// insert adds c without the reserved flag check, for commands that have already passed
+// it on another registry.
+func (r *Registry) insert(c *Command) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	r.commands[c.Name] = c
@@ -79,7 +190,8 @@ func (r *Registry) Match(prefix string) []string {
 	return cmds
 }
 
-// All returns every registered command sorted by name, which is the order help lists them.
+// All returns every registered command sorted by name, which is the order help lists
+// them and the schema describes them.
 func (r *Registry) All() []*Command {
 	r.mu.RLock()
 	defer r.mu.RUnlock()
@@ -89,35 +201,37 @@ func (r *Registry) All() []*Command {
 	return cmds
 }
 
-// Clone returns a copy holding the same commands, with help bound to the copy: a
-// command registered on the clone alone is listed and described there, and one the
-// clone withholds is not. That rebinding replaces whatever was registered under Help on
-// the source, built-in or not.
+// Clone returns a copy holding the same commands.
+//
+// help is re-registered on the copy rather than carried over, because the source's help
+// is bound to the source and would list its commands, not the copy's, once a caller
+// registers something on the copy.
 func (r *Registry) Clone() *Registry {
 	r.mu.RLock()
-	c := &Registry{commands: maps.Clone(r.commands), full: r.full, reason: r.reason}
+	out := &Registry{
+		commands:      maps.Clone(r.commands),
+		full:          r.full,
+		reason:        r.reason,
+		DefaultFormat: r.DefaultFormat,
+	}
 	r.mu.RUnlock()
-	c.registerHelp()
-	return c
-}
 
-// ErrWithheld is returned when a command exists but this surface does not serve it.
-var ErrWithheld = errors.New("command unavailable")
+	out.registerHelp()
+	return out
+}
 
-// Filter returns a copy holding the commands keep returns true for.
+// Filter returns a filtered copy of the registry.
 //
-// A surface decides for itself what it will serve; the registry only applies the
-// decision. Filtering here, on the side that serves requests, is what stops a client
-// reaching a withheld command by naming it directly.
+// Keep should return true for commands that should be included in the copy.
 //
 // reason says, to an operator who names a command the copy left out, why this surface
 // does not serve it; "" leaves the registry to say only that it does not.
 func (r *Registry) Filter(keep func(*Command) bool, reason string) *Registry {
-	out := &Registry{commands: map[Name]*Command{}, full: r, reason: reason}
+	out := &Registry{commands: map[Name]*Command{}, full: r, reason: reason, DefaultFormat: r.DefaultFormat}
 
 	for _, cmd := range r.All() {
 		if keep(cmd) {
-			out.Register(cmd)
+			out.insert(cmd)
 		}
 	}
 
@@ -127,14 +241,14 @@ func (r *Registry) Filter(keep func(*Command) bool, reason string) *Registry {
 }
 
 // Dispatch parses a command line and runs it, reporting problems to w. The error is for
-// the caller to log, not to show; see CommandCallback.
-func (r *Registry) Dispatch(line string, w StringWriter) error {
+// the caller to log, not to show; see RunFunc and Failure.
+func (r *Registry) Dispatch(line string, w Output) error {
 	// A line shlex cannot split, an unbalanced quote. Reported on Err so the output
 	// stream stays parseable.
 	args, err := shlex.Split(line, true)
 	if err != nil {
 		msg := "could not parse the command line: " + err.Error()
-		if _, err := fmt.Fprintln(w.Err(), msg); err != nil {
+		if _, err := fmt.Fprintln(w.Err, msg); err != nil {
 			return err
 		}
 		return &Failure{Msg: msg}
@@ -144,95 +258,59 @@ func (r *Registry) Dispatch(line string, w StringWriter) error {
 }
 
 // DispatchArgs runs an already tokenized command line. The error is as for Dispatch.
-func (r *Registry) DispatchArgs(args []string, w StringWriter) error {
+//
+// An empty line and an unknown command both end up running `help`, the first with the
+// arguments help itself would take; `<command> -h` is answered by the command's own
+// FlagSet, with the same rendering, in execLine.
+func (r *Registry) DispatchArgs(args []string, w Output) error {
 	if len(args) == 0 {
-		r.dumpCommands(w)
-		return nil
+		args = []string{"help"}
 	}
 
 	c := r.Get(Name(args[0]))
 	if c == nil {
-		if withheld := r.withheld(Name(args[0])); withheld != nil {
+		if err := r.unavailable(Name(args[0])); err != nil {
 			// Saying why, rather than "did not understand", keeps the rule legible and
 			// points at a surface that will actually run the command.
-			_, _ = fmt.Fprintf(w.Err(), "%s is not available on this surface: %s\n", withheld.Name, r.withheldReason(withheld))
-			return ErrWithheld
+			_, _ = fmt.Fprintln(w.Err, err)
+			return err
 		}
 
 		// Only the first word was looked up, so only it is named.
 		msg := fmt.Sprintf("did not understand: %s", args[0])
-		if err := w.WriteLine(msg); err != nil {
+		if _, err := fmt.Fprintln(w.Out, msg); err != nil {
 			return err
 		}
 
-		r.dumpCommands(w)
+		if err := r.DispatchArgs([]string{"help"}, w); err != nil {
+			return err
+		}
 		return &Failure{Msg: msg}
 	}
 
-	if IsHelpRequest(args) {
-		// Through the registered help command, so -h and `help <name>` are one path.
-		return execCommand(r.Get(Help), []string{string(c.Name)}, w)
-	}
-
-	return execCommand(c, args[1:], w)
+	return r.execLine(c, args[1:], w)
 }
 
-// dumpCommands writes the list of available commands to w.
-func (r *Registry) dumpCommands(w StringWriter) {
-	err := w.WriteLine("Available commands:")
-	if err != nil {
-		return
-	}
-
-	// In the order All gives, which is the display order.
-	cmds := make([]string, 0)
-	for _, l := range r.All() {
-		cmds = append(cmds, fmt.Sprintf("%s - %s", l.Name, l.ShortDescription))
-	}
-
-	_ = w.Write(strings.Join(cmds, "\n") + "\n\n")
-}
-
-// help writes the command list, or usage for the single command named in a.
-func (r *Registry) help(a []string, w StringWriter) error {
-	// Just typed help
-	if len(a) == 0 {
-		r.dumpCommands(w)
+// unavailable explains why name cannot be run here when this registry was filtered from
+// one that holds it: the command exists, and this surface withholds it, for the reason
+// whoever built the filter gave. It is nil when name is simply unknown, so the two cases
+// read differently to an operator.
+func (r *Registry) unavailable(name Name) error {
+	if r.full == nil {
 		return nil
 	}
 
-	// We are printing a specific commands help text
-	cmd := r.Get(Name(a[0]))
-	if cmd == nil {
-		if withheld := r.withheld(Name(a[0])); withheld != nil {
-			msg := fmt.Sprintf("%s is not available on this surface: %s", withheld.Name, r.withheldReason(withheld))
-			if _, err := fmt.Fprintln(w.Err(), msg); err != nil {
-				return err
-			}
-			return &Failure{Msg: msg}
-		}
-		return Fail(w, "Command not available "+a[0])
-	}
-
-	if err := w.WriteLine(fmt.Sprintf("%s - %s", cmd.Name, cmd.ShortDescription)); err != nil {
-		return err
-	}
-
-	if cmd.Help != "" {
-		if err := w.WriteLine(fmt.Sprintf("  %s", cmd.Help)); err != nil {
-			return err
-		}
+	c := r.full.Get(name)
+	if c == nil {
+		return nil
 	}
 
-	if cmd.Flags != nil {
-		fs, _ := cmd.Flags()
-		if fs != nil {
-			fs.SetOutput(w.Err())
-			fs.PrintDefaults()
-		}
+	reason := r.reason
+	if reason == "" {
+		reason = "this surface does not serve it"
 	}
 
-	return nil
+	return fmt.Errorf("%s is not available on this surface: %s", c.Name, reason)
 }
 
 // Resolve returns the command registered under name, or says why there is none. A
@@ -245,41 +323,92 @@ func (r *Registry) Resolve(name Name) (*Command, error) {
 		return c, nil
 	}
 
-	if withheld := r.withheld(name); withheld != nil {
-		return nil, fmt.Errorf("%s is not available on this surface: %s", withheld.Name, r.withheldReason(withheld))
+	if err := r.unavailable(name); err != nil {
+		return nil, err
 	}
 
 	return nil, fmt.Errorf("unknown command %q", name)
 }
 
-// Schema describes every command in the registry, so a client can offer them without
-// having been built against this daemon.
-func (r *Registry) Schema() []CommandSchema {
-	// In the order All gives, which is the display order.
-	out := make([]CommandSchema, 0, len(r.All()))
-
-	for _, c := range r.All() {
-		cs := CommandSchema{Name: c.Name, Short: c.ShortDescription, Help: c.Help}
-
-		if c.Flags != nil {
-			if fl, _ := c.Flags(); fl != nil {
-				fl.VisitAll(func(f *flag.Flag) {
-					cs.Flags = append(cs.Flags, FlagSchema{
-						Name:    f.Name,
-						Bool:    isBoolFlag(f),
-						Default: f.DefValue,
-						Usage:   f.Usage,
-					})
-				})
+// setParams sets each named parameter on fl, in the order of names, and stops at the
+// first that is unknown or does not parse. own says whether the command declared flags
+// of its own, which decides between "takes no parameters" and "has no parameter".
+//
+// The presentation flags are on fl, since commandFlags put them there, but they are not
+// parameters: a request has one place to choose its rendering, the envelope's format
+// field, and a client that sends them here is told where that is rather than having a
+// second, weaker way to say the same thing.
+func setParams(fl *flag.FlagSet, name string, names []string, params map[string]string, own bool) error {
+	for _, k := range names {
+		if isPresentationFlag(k) {
+			return fmt.Errorf("%s is a command line flag, not a parameter; choose the rendering with the request's format field", k)
+		}
+
+		f := fl.Lookup(k)
+		if f == nil {
+			if !own {
+				return fmt.Errorf("%s takes no parameters", name)
 			}
+			return fmt.Errorf("%s has no parameter %q", name, k)
 		}
 
-		out = append(out, cs)
+		v := params[k]
+		if err := fl.Set(k, v); err != nil {
+			if IsBoolFlag(f) {
+				// The flag package's own words for this are "parse error".
+				return fmt.Errorf("%s: expected a boolean, got %q", k, v)
+			}
+			return fmt.Errorf("%s: %w", k, err)
+		}
 	}
 
+	return nil
+}
+
+// Schema describes every command in the registry, so a client can offer them without
+// having been built against this daemon.
+func (r *Registry) Schema() []CommandSchema {
+	all := r.All()
+	out := make([]CommandSchema, 0, len(all))
+	for _, c := range all {
+		out = append(out, r.schemaFor(c))
+	}
 	return out
 }
 
+// schemaFor describes one command. It is the one derivation, for the whole schema and
+// for `help <command>` alike.
+func (r *Registry) schemaFor(c *Command) CommandSchema {
+	fl, _, _ := commandFlags(c)
+	return schemaOf(c, fl)
+}
+
+// schemaOf describes c as driven through fl, the FlagSet commandFlags built for it. The
+// presentation flags are included because a command line client parses its line
+// against this and help renders from it; they are listed after the command's own,
+// which is where readers expect them. Invoke refuses them as parameters.
+func schemaOf(c *Command, fl *flag.FlagSet) CommandSchema {
+	cs := CommandSchema{Name: c.Name, Short: c.ShortDescription, Help: c.Help}
+
+	var own, shared []FlagSchema
+	fl.VisitAll(func(f *flag.Flag) {
+		fs := FlagSchema{
+			Name:    f.Name,
+			Bool:    IsBoolFlag(f),
+			Default: f.DefValue,
+			Usage:   f.Usage,
+		}
+		if isPresentationFlag(f.Name) {
+			shared = append(shared, fs)
+			return
+		}
+		own = append(own, fs)
+	})
+	cs.Flags = append(own, shared...)
+
+	return cs
+}
+
 // CommandSchema describes one command well enough for a client to offer it.
 type CommandSchema struct {
 	Name  Name         `json:"name"`
@@ -299,30 +428,3 @@ type FlagSchema struct {
 	Default string `json:"default,omitempty"`
 	Usage   string `json:"usage,omitempty"`
 }
-
-// isBoolFlag reports whether f is satisfied by its presence alone. The flag package marks
-// these with an unexported interface, which is the only way to tell.
-func isBoolFlag(f *flag.Flag) bool {
-	bf, ok := f.Value.(interface{ IsBoolFlag() bool })
-	return ok && bf.IsBoolFlag()
-}
-
-// withheld returns the command named name if it exists in the unfiltered registry but
-// not in this one.
-func (r *Registry) withheld(name Name) *Command {
-	if r.full == nil {
-		return nil
-	}
-
-	return r.full.Get(name)
-}
-
-// withheldReason renders the explanation supplied to Filter, falling back to something
-// true but unhelpful when no explanation was given.
-func (r *Registry) withheldReason(c *Command) string {
-	if r.reason == "" {
-		return "this surface does not serve it"
-	}
-
-	return r.reason
-}

commands/registry_test.go

@@ -2,10 +2,12 @@ package commands
 
 import (
 	"bytes"
-	"errors"
 	"flag"
+	"io"
 	"strings"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -17,22 +19,30 @@ func notWithheld(c *Command) bool { return !c.ForProfiling }
 
 const testReason = "this surface does not run profiling commands"
 
+// textLine is a Text renderer for test commands whose result is one string.
+func textLine(v any, w io.Writer) error {
+	_, err := io.WriteString(w, v.(string)+"\n")
+	return err
+}
+
 func testRegistry() *Registry {
 	r := New()
 	r.Register(&Command{
 		Name:             "served",
 		ShortDescription: "a command this surface serves",
-		Callback: func(fs any, a []string, w StringWriter) error {
-			return w.WriteLine("read " + strings.Join(a, ","))
+		Run: func(fs any, a []string) (Result, error) {
+			return Value("read " + strings.Join(a, ",")), nil
 		},
+		Text: textLine,
 	})
 	r.Register(&Command{
 		Name:             "withheld",
 		ShortDescription: "a command this surface withholds",
 		ForProfiling:     true,
-		Callback: func(fs any, a []string, w StringWriter) error {
-			return w.WriteLine("wrote")
+		Run: func(fs any, a []string) (Result, error) {
+			return Value("wrote"), nil
 		},
+		Text: textLine,
 	})
 	return r
 }
@@ -55,9 +65,7 @@ func TestUnannotatedCommandIsServed(t *testing.T) {
 	r.Register(&Command{
 		Name:             "plain",
 		ShortDescription: "a command with no annotations",
-		Callback: func(fs any, a []string, w StringWriter) error {
-			return nil
-		},
+		Run:              func(fs any, a []string) (Result, error) { return Nothing(), nil },
 	})
 
 	cmd := r.Filter(notWithheld, testReason).Get("plain")
@@ -70,9 +78,7 @@ func TestProfilingCommandIsWithheld(t *testing.T) {
 		Name:             "save-a-profile",
 		ShortDescription: "drives the profiler",
 		ForProfiling:     true,
-		Callback: func(fs any, a []string, w StringWriter) error {
-			return nil
-		},
+		Run:              func(fs any, a []string) (Result, error) { return Nothing(), nil },
 	})
 
 	cmd := r.Filter(notWithheld, testReason).Get("save-a-profile")
@@ -84,11 +90,12 @@ func TestProfilingCommandIsWithheld(t *testing.T) {
 // the client was told was available.
 func TestRefusesWithheldByName(t *testing.T) {
 	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
-	w := NewSplitStringWriter(out, errOut)
+	w := Output{Out: out, Err: errOut}
 
 	err := testRegistry().Filter(notWithheld, testReason).DispatchArgs([]string{"withheld"}, w)
 
-	require.ErrorIs(t, err, ErrWithheld)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "withheld is not available on this surface")
 	assert.Empty(t, out.String(), "a withheld command must produce no output")
 	assert.Contains(t, errOut.String(), "withheld is not available on this surface")
 	assert.Contains(t, errOut.String(), testReason,
@@ -99,10 +106,10 @@ func TestRefusesWithheldByName(t *testing.T) {
 // mistypes and one who asked for a real command should not get the same answer.
 func TestWithheldIsNotReportedAsUnknown(t *testing.T) {
 	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
-	err := testRegistry().Filter(notWithheld, testReason).DispatchArgs([]string{"no-such-command"}, NewSplitStringWriter(out, errOut))
+	err := testRegistry().Filter(notWithheld, testReason).DispatchArgs([]string{"no-such-command"}, Output{Out: out, Err: errOut})
 
 	var f *Failure
-	require.ErrorAs(t, err, &f)
+	require.ErrorAs(t, err, &f, "an unknown command is a failure to the surface")
 	assert.Contains(t, out.String(), "did not understand: no-such-command")
 }
 
@@ -115,188 +122,339 @@ func TestSplitWriterKeepsOutputParseable(t *testing.T) {
 		ShortDescription: "emits json",
 		Flags: func() (*flag.FlagSet, any) {
 			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			fl.Bool("json", false, "emit json")
+			fl.Bool("verbose", false, "say more")
 			return fl, nil
 		},
-		Callback: func(fs any, a []string, w StringWriter) error {
-			return w.Write(`{"ok":true}`)
+		Run: func(fs any, a []string) (Result, error) {
+			return Value(map[string]bool{"ok": true}), nil
 		},
 	})
 
 	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
-	require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, NewSplitStringWriter(out, errOut)))
+	require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, Output{Out: out, Err: errOut}))
 
 	assert.Empty(t, out.String(), "flag errors must not land on the output stream")
 	assert.Contains(t, errOut.String(), "flag provided but not defined")
 }
 
-// TestFlagUsageGoesToTheWriter pins that a parse failure's usage lands on the command's
-// writer.
-func TestFlagUsageGoesToTheWriter(t *testing.T) {
+// TestMergedWriterPreservesSshBehavior: the ssh surface points both streams at one
+// channel, and must keep seeing a single merged stream.
+func TestMergedWriterPreservesSshBehavior(t *testing.T) {
 	r := New()
 	r.Register(&Command{
 		Name:             "emit",
 		ShortDescription: "emits",
 		Flags: func() (*flag.FlagSet, any) {
 			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			fl.Bool("json", false, "emit json")
+			fl.Bool("verbose", false, "say more")
 			return fl, nil
 		},
-		Callback: func(fs any, a []string, w StringWriter) error { return nil },
+		Run: func(fs any, a []string) (Result, error) { return Nothing(), nil },
 	})
 
 	merged := &bytes.Buffer{}
-	require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, NewStringWriter(merged)))
+	require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, MergedOutput(merged)))
 	assert.Contains(t, merged.String(), "flag provided but not defined")
 }
 
+// paramRegistry holds one command with a bool flag and a duration flag, and reports what
+// the command saw when it ran.
+func paramRegistry(t *testing.T) (*Registry, *[]paramsSeen) {
+	t.Helper()
+
+	var seen []paramsSeen
+	r := New()
+	r.Register(&Command{
+		Name:             "set",
+		ShortDescription: "sets things",
+		Flags: func() (*flag.FlagSet, any) {
+			fl := flag.NewFlagSet("", flag.ContinueOnError)
+			s := paramsSeen{}
+			fl.BoolVar(&s.Verbose, "verbose", false, "say more")
+			fl.DurationVar(&s.Wait, "wait", 0, "how long")
+			return fl, &s
+		},
+		Run: func(fs any, a []string) (Result, error) {
+			seen = append(seen, *fs.(*paramsSeen))
+			return Value("ok"), nil
+		},
+	})
+
+	return r, &seen
+}
+
+type paramsSeen struct {
+	Verbose bool
+	Wait    time.Duration
+}
+
+// TestInvokeReportsParametersInNameOrder: parameters arrive as an unordered object, so
+// with two bad ones the error must not depend on map iteration. Name order decides.
+func TestInvokeReportsParametersInNameOrder(t *testing.T) {
+	r, _ := paramRegistry(t)
+
+	cases := map[string]struct {
+		params map[string]string
+		want   string
+	}{
+		"two unknown":               {map[string]string{"zed": "1", "alpha": "1"}, `set has no parameter "alpha"`},
+		"unknown before bad bool":   {map[string]string{"aaa": "1", "verbose": "maybe"}, `set has no parameter "aaa"`},
+		"bad bool before unknown":   {map[string]string{"verbose": "maybe", "zzz": "1"}, `verbose: expected a boolean, got "maybe"`},
+		"bad value before bad bool": {map[string]string{"wait": "soon", "verbose": "maybe"}, `verbose: expected a boolean`},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			// Enough runs that a map-order dependence would show.
+			for i := 0; i < 50; i++ {
+				_, err := Invoke(Request{Command: r.Get("set"), Params: tc.params})
+				require.Error(t, err)
+				assert.ErrorContains(t, err, tc.want)
+			}
+		})
+	}
+}
+
+// TestInvokeWithABadParameterSetsNothing: a request with any bad parameter must not run
+// the command, and must leave nothing behind for the next request to see. The good
+// parameter sorts before the bad one, so a one-pass Invoke would have applied it.
+func TestInvokeWithABadParameterSetsNothing(t *testing.T) {
+	r, seen := paramRegistry(t)
+
+	_, err := Invoke(Request{Command: r.Get("set"), Params: map[string]string{"verbose": "true", "wait": "soon"}})
+	require.ErrorContains(t, err, "wait:")
+	assert.Empty(t, *seen, "the command must not run with a partial parameter set")
+
+	v, err := Invoke(Request{Command: r.Get("set")})
+	require.NoError(t, err)
+	assert.Equal(t, "ok", v.Value)
+	require.Len(t, *seen, 1)
+	assert.Equal(t, paramsSeen{}, (*seen)[0], "nothing from the failed request carried over")
+
+	_, err = Invoke(Request{Command: r.Get("set"), Params: map[string]string{"verbose": "true", "wait": "2s"}})
+	require.NoError(t, err)
+	require.Len(t, *seen, 2)
+	assert.Equal(t, paramsSeen{Verbose: true, Wait: 2 * time.Second}, (*seen)[1])
+}
+
 func TestHelpListsOnlyServedCommands(t *testing.T) {
 	out := &bytes.Buffer{}
-	require.NoError(t, testRegistry().Filter(notWithheld, testReason).Dispatch("help", NewStringWriter(out)))
+	require.NoError(t, testRegistry().Filter(notWithheld, testReason).DispatchArgs(nil, MergedOutput(out)))
 
 	assert.Contains(t, out.String(), "served")
 	assert.NotContains(t, out.String(), "withheld")
 }
 
-// TestDispatchReportsUnparseableLine: a line shlex rejects, an unbalanced quote, is a
-// failure the user is told about.
-func TestDispatchReportsUnparseableLine(t *testing.T) {
+func TestDispatchHelpFlag(t *testing.T) {
 	out := &bytes.Buffer{}
-	err := testRegistry().Dispatch(`served "unterminated`, NewStringWriter(out))
-
-	var f *Failure
-	require.ErrorAs(t, err, &f)
-	assert.Equal(t, f.Msg+"\n", out.String())
-	assert.Contains(t, out.String(), "could not parse the command line: ")
+	testRegistry().Dispatch("served -h", MergedOutput(out))
+	assert.Contains(t, out.String(), "served - a command this surface serves")
 }
 
-// TestDispatchReportsUnparseableLineOnErr: the refusal lands on Err, so a surface that
-// separates the streams keeps its output parseable.
-func TestDispatchReportsUnparseableLineOnErr(t *testing.T) {
+// TestDispatchReportsUnparseableLine pins that a line shlex rejects says so on Err rather
+// than coming back silent, so an operator with an unbalanced quote learns the quote is the
+// problem instead of assuming the command is broken.
+func TestDispatchReportsUnparseableLine(t *testing.T) {
 	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
-	_ = testRegistry().Dispatch(`served "unterminated`, NewSplitStringWriter(out, errOut))
+	testRegistry().Dispatch(`served "unterminated`, Output{Out: out, Err: errOut})
 	assert.Empty(t, out.String())
 	assert.Contains(t, errOut.String(), "could not parse the command line")
 }
 
-// TestFailTellsTheUserAndTheSurface: Fail writes the message and returns it as a Failure;
-// when the write fails, the user heard nothing, so the write error is what comes back.
-func TestFailTellsTheUserAndTheSurface(t *testing.T) {
-	out := &bytes.Buffer{}
-	err := Fail(NewStringWriter(out), "no such tunnel")
-	var f *Failure
-	require.ErrorAs(t, err, &f)
-	assert.Equal(t, "no such tunnel", f.Msg)
-	assert.Equal(t, "no such tunnel\n", out.String())
-
-	err = Fail(NewStringWriter(failingWriter{}), "no such tunnel")
-	require.EqualError(t, err, "gone")
-	assert.NotErrorAs(t, err, &f)
+// 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.
+func TestRegisterRejectsReservedFlags(t *testing.T) {
+	for _, name := range []string{"json", "pretty", "h", "help", "config", "socket"} {
+		t.Run(name, func(t *testing.T) {
+			r := New()
+			assert.PanicsWithValue(t, "commands: clash declares reserved flag -"+name, func() {
+				r.Register(&Command{
+					Name: "clash",
+					Flags: func() (*flag.FlagSet, any) {
+						fl := flag.NewFlagSet("clash", flag.ContinueOnError)
+						fl.Bool(name, false, "")
+						return fl, nil
+					},
+					Run: func(fs any, a []string) (Result, error) { return Nothing(), nil },
+				})
+			})
+		})
+	}
 }
 
-type failingWriter struct{}
+func TestCloneDoesNotAffectOriginal(t *testing.T) {
+	r := testRegistry()
+	c := r.Clone()
+	c.Register(&Command{Name: "logout", ShortDescription: "ends the session", Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }})
 
-func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("gone") }
+	assert.Nil(t, r.Get("logout"))
+}
 
-// TestAllSortsByName: registration order does not matter.
-func TestAllSortsByName(t *testing.T) {
-	r := New()
-	for _, n := range []Name{"zz", Version, "aa", ChangeRemote} {
-		r.Register(&Command{Name: n})
+// TestRegistryIsSafeForConcurrentUse runs the operations the surfaces perform, filtering
+// and cloning per request or session, against a stream of registrations. It exists to
+// be run under the race detector: the map underneath is not goroutine safe.
+func TestRegistryIsSafeForConcurrentUse(t *testing.T) {
+	r := testRegistry()
+
+	var wg sync.WaitGroup
+	for i := 0; i < 8; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			for j := 0; j < 50; j++ {
+				r.Register(&Command{
+					Name:             Name("late-" + strings.Repeat("a", i) + "-" + strings.Repeat("b", j)),
+					ShortDescription: "registered after the surfaces started",
+					Run:              func(fs any, a []string) (Result, error) { return Nothing(), nil },
+				})
+			}
+		}(i)
 	}
 
-	var got []Name
-	for _, c := range r.All() {
-		got = append(got, c.Name)
+	for i := 0; i < 8; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for j := 0; j < 50; j++ {
+				f := r.Filter(notWithheld, testReason)
+				_ = f.Get("served")
+				_ = f.Schema()
+				_ = r.Clone().Match("late-")
+			}
+		}()
 	}
-	assert.Equal(t, []Name{"aa", ChangeRemote, Help, Version, "zz"}, got)
+
+	wg.Wait()
+
+	f := r.Filter(notWithheld, testReason)
+	assert.Len(t, f.Match("late-"), 8*50, "every late registration is visible once the writers finish")
 }
 
-// TestHelpFlagAgreesWithHelpCommand pins that -h answers through the registered help
-// command, and that help on a clone describes the clone: a command registered only there
-// is known to both -h and `help <name>`, and one withheld from it is known to neither.
+// TestHelpFlagAgreesWithHelpCommand pins that -h on a command and `help <name>` print the
+// same text, on a clone as on the source: both render through WriteCommandHelp, and Clone
+// rebinds help to the copy, so a command registered on the clone is found either way.
 func TestHelpFlagAgreesWithHelpCommand(t *testing.T) {
-	r := testRegistry()
-	c := r.Clone()
+	c := testRegistry().Clone()
 	c.Register(&Command{
 		Name:             Logout,
 		ShortDescription: "ends the session",
-		Callback:         func(fs any, a []string, w StringWriter) error { return nil },
+		Run:              func(fs any, a []string) (Result, error) { return Nothing(), nil },
 	})
 
-	flag := &bytes.Buffer{}
-	c.Dispatch("logout -h", NewStringWriter(flag))
-	cmd := &bytes.Buffer{}
-	c.Dispatch("help logout", NewStringWriter(cmd))
-	assert.Equal(t, cmd.String(), flag.String())
-	assert.Equal(t, "logout - ends the session\n", flag.String())
-
-	// The source never learns of it.
-	src := &bytes.Buffer{}
-	r.Dispatch("help logout", NewStringWriter(src))
-	assert.Equal(t, "Command not available logout\n", src.String())
-
-	// And a command both hold is described either way.
-	flag.Reset()
-	c.Dispatch("served -h", NewStringWriter(flag))
-	assert.Equal(t, "served - a command this surface serves\n", flag.String())
+	for _, name := range []string{"logout", "served"} {
+		flag := &bytes.Buffer{}
+		c.Dispatch(name+" -h", MergedOutput(flag))
+		cmd := &bytes.Buffer{}
+		c.Dispatch("help "+name, MergedOutput(cmd))
+		assert.Equal(t, cmd.String(), flag.String(), name)
+		assert.Contains(t, flag.String(), name+" - ", name)
+	}
 }
 
-// TestCloneHelpListsTheClone: the command list help prints is the clone's, so a clone
-// that withholds a command does not name it, and the source still does.
-func TestCloneHelpListsTheClone(t *testing.T) {
-	r := testRegistry()
-	c := New()
-	c.Register(r.Get("served"))
-	c = c.Clone()
+// TestCloneHelpSeesCloneCommands: help on a clone answers for the clone. The ssh session
+// registers logout on its clone, and `help logout` has to find it.
+func TestCloneHelpSeesCloneCommands(t *testing.T) {
+	c := testRegistry().Clone()
+	c.Register(&Command{Name: "logout", ShortDescription: "ends the session", Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }})
 
 	out := &bytes.Buffer{}
-	c.Dispatch("help", NewStringWriter(out))
-	assert.Equal(t, "Available commands:\n"+
-		"help - prints available commands or help <command> for specific usage info\n"+
-		"served - a command this surface serves\n\n", out.String())
+	require.NoError(t, c.DispatchArgs([]string{"help", "logout"}, MergedOutput(out)))
+	assert.Contains(t, out.String(), "ends the session")
 
 	out.Reset()
-	c.Dispatch("help withheld", NewStringWriter(out))
-	assert.Equal(t, "Command not available withheld\n", out.String())
-
-	out.Reset()
-	r.Dispatch("help withheld", NewStringWriter(out))
-	assert.Equal(t, "withheld - a command this surface withholds\n", out.String())
+	require.NoError(t, c.DispatchArgs([]string{"help"}, MergedOutput(out)))
+	assert.Contains(t, out.String(), "logout")
 }
 
-// TestDispatchReturnsTheCommandsError: what the callback returns reaches the caller, so
-// a surface can log it.
-func TestDispatchReturnsTheCommandsError(t *testing.T) {
+// TestAfterReplyRunsAfterTheReply pins the order AfterReply exists for: the reply is on
+// the writer before the action begins.
+func TestAfterReplyRunsAfterTheReply(t *testing.T) {
+	out := &bytes.Buffer{}
+	seen := "never ran"
+
 	r := New()
 	r.Register(&Command{
-		Name: "fails",
-		Callback: func(fs any, a []string, w StringWriter) error {
-			return errors.New("boom")
+		Name:             "act",
+		ShortDescription: "replies, then acts",
+		Run: func(fs any, a []string) (Result, error) {
+			return Did("about to act"), nil
 		},
+		AfterReply: func() { seen = out.String() },
 	})
 
-	out := &bytes.Buffer{}
-	require.EqualError(t, r.Dispatch("fails", NewStringWriter(out)), "boom")
-	assert.Empty(t, out.String(), "a command's own error is not shown; the command speaks for itself")
+	require.NoError(t, r.DispatchArgs([]string{"act"}, MergedOutput(out)))
+	assert.Contains(t, seen, "about to act")
+	assert.Equal(t, out.String(), seen)
 }
 
-// TestUnknownCommandNamesTheWord: only the first word is looked up, so only it is
-// reported, followed by the command list, and the line is a failure to the surface.
-func TestUnknownCommandNamesTheWord(t *testing.T) {
+// TestWriteCommandHelpIsTheOneRendering pins the layout both `help <command>` on the
+// daemon and `-h` in the client print, so the two cannot drift apart again.
+func TestWriteCommandHelpIsTheOneRendering(t *testing.T) {
+	cs := CommandSchema{
+		Name:  "change-remote",
+		Short: "changes a remote",
+		Help:  "Give it an address.",
+		Flags: []FlagSchema{
+			{Name: "address", Usage: "the new remote", Default: "1.2.3.4:4242"},
+			{Name: "by-index", Bool: true, Usage: "match by index"},
+			{Name: "json", Bool: true, Usage: "outputs as json"},
+		},
+	}
+
 	out := &bytes.Buffer{}
-	err := testRegistry().Dispatch(`nope "with args"`, NewStringWriter(out))
-	var f *Failure
-	require.ErrorAs(t, err, &f)
-	assert.Equal(t, "did not understand: nope", f.Msg)
-	assert.True(t, strings.HasPrefix(out.String(), "did not understand: nope\nAvailable commands:\n"), out.String())
+	require.NoError(t, WriteCommandHelp(cs, out))
+
+	assert.Equal(t, "change-remote - changes a remote\n"+
+		"  Give it an address.\n"+
+		"  -address string\n    \tthe new remote (default 1.2.3.4:4242)\n"+
+		"  -by-index\n    \tmatch by index\n"+
+		"  -json\n    \toutputs as json\n", out.String())
 }
 
-func TestIsHelpRequestAcceptsEveryHelpSpelling(t *testing.T) {
-	for _, args := range [][]string{{"-h"}, {"-help"}, {"--help"}, {"x", "-h"}} {
-		assert.True(t, IsHelpRequest(args), "%v", args)
+// addrFlags is the flag struct of a test command that takes -address.
+type addrFlags struct{ Address string }
+
+// TestHelpFlagIsReadWhereParseReadsIt: -h asks for help only where the line's parser
+// reads it as a flag. As a flag's value or after -- it reaches the command as the
+// operand it is, and help is rendered by the same function `help <command>` uses.
+func TestHelpFlagIsReadWhereParseReadsIt(t *testing.T) {
+	var seen []string
+	r := New()
+	r.Register(Flagged(&Command{Name: "x", ShortDescription: "takes an address"},
+		func(fl *flag.FlagSet, s *addrFlags) { fl.StringVar(&s.Address, "address", "", "an address") },
+		func(s *addrFlags, a []string) (Result, error) {
+			seen = append(seen, s.Address+"|"+strings.Join(a, ","))
+			return Nothing(), nil
+		}))
+
+	for _, line := range []string{"x -h", "x -help", "x --help", "x 10.0.0.1 -h", "x -address=1 -h"} {
+		out := &bytes.Buffer{}
+		r.Dispatch(line, MergedOutput(out))
+		assert.Contains(t, out.String(), "x - takes an address\n", line)
+		assert.Contains(t, out.String(), "-address string", line)
 	}
-	for _, args := range [][]string{nil, {}, {"help"}, {"-hx"}, {"x"}} {
-		assert.False(t, IsHelpRequest(args), "%v", args)
+	assert.Empty(t, seen, "asking for help must not run the command")
+
+	for _, line := range []string{"x -address -h", "x -- -h"} {
+		out := &bytes.Buffer{}
+		r.Dispatch(line, MergedOutput(out))
+		assert.Empty(t, out.String(), line)
 	}
+	assert.Equal(t, []string{"-h|", "|-h"}, seen)
+}
+
+// TestBadFlagIsAnsweredWithTheCommandsHelp: a flag the command does not have is reported
+// on the error stream followed by the command's usage, in the rendering help uses.
+func TestBadFlagIsAnsweredWithTheCommandsHelp(t *testing.T) {
+	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
+	r := New()
+	r.Register(&Command{Name: "x", ShortDescription: "has no flags", Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }})
+
+	require.Error(t, r.DispatchArgs([]string{"x", "-nope"}, Output{Out: out, Err: errOut}))
+	assert.Empty(t, out.String())
+	assert.Equal(t, "flag provided but not defined: -nope\nx - has no flags\n  -json\n    \toutputs as json\n  -pretty\n    \tpretty prints json, assumes -json\n", errOut.String())
 }

commands/render.go

@@ -0,0 +1,182 @@
+package commands
+
+import (
+	"encoding/json"
+	"errors"
+	"fmt"
+	"io"
+)
+
+// ErrNoTextRendering is returned by Render when a command's Value has no human rendering
+// and text was asked for anyway. A command declares this by leaving Text nil and saying
+// so with JSONOnly; it is not a fallback situation but a statement that no such form
+// exists yet, so nobody builds a script around an interim one. Each surface's default
+// policy avoids ever asking.
+var ErrNoTextRendering = errors.New("no human readable output yet; pass -json or -pretty")
+
+// Format is how a result should be presented.
+type Format int
+
+const (
+	// FormatUnset is a request that expressed no preference. A surface resolves it with
+	// its own default before rendering; Render itself treats it as FormatText.
+	FormatUnset Format = iota
+
+	// FormatText renders for a person. It is what an operator gets by default on every
+	// surface with a human at the other end.
+	FormatText
+
+	// FormatJSON renders the result as compact JSON.
+	FormatJSON
+
+	// FormatPrettyJSON renders the result as indented JSON.
+	FormatPrettyJSON
+)
+
+// Flag names for the two presentation flags. They are offered on every command rather
+// than declared by each one, because how a result is displayed is not a property of the
+// command that produced it.
+const (
+	FlagJSON   = "json"
+	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.
+const (
+	FlagConfig = "config"
+	FlagSocket = "socket"
+)
+
+// reservedFlags are handled before a command's Run is called, so a command may not
+// declare them itself. -h and -help are intercepted as a request for usage; -json and
+// -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.
+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.
+func Render(c *Command, r Result, f Format, w io.Writer) error {
+	switch f {
+	case FormatJSON:
+		return renderJSON(r.Payload(), w, false)
+
+	case FormatPrettyJSON:
+		return renderJSON(r.Payload(), w, true)
+
+	default:
+		// An outcome reads the same for every command: its reason, as a sentence. Only a
+		// Value needs the command's own rendering, so Text never sees anything else.
+		if r.Value == nil {
+			_, err := io.WriteString(w, r.Reason+"\n")
+			return err
+		}
+
+		if c != nil && c.Text != nil {
+			return c.Text(r.Value, w)
+		}
+
+		name := "this command"
+		if c != nil {
+			name = string(c.Name)
+		}
+		return fmt.Errorf("%s has %w", name, ErrNoTextRendering)
+	}
+}
+
+func renderJSON(v any, w io.Writer, indent bool) error {
+	enc := json.NewEncoder(w)
+	if indent {
+		enc.SetIndent("", "    ")
+	}
+
+	return enc.Encode(v)
+}
+
+// Result is what every Run returns: whether the command worked, a sentence saying so,
+// and, for a command that reports rather than acts, the value it produced.
+//
+// Value and Reason are alternatives. A command that reports returns Value(v), and the
+// surfaces render v, as JSON or through the command's Text. A command that acts, or any
+// command refusing its arguments, returns Did or Failed, and the surfaces render the
+// Outcome: its Reason is the sentence these commands have always printed. Nothing is for
+// a command with nothing to say.
+//
+// The envelope itself never reaches a caller. Payload is what JSON carries, and Render
+// hands Text only the Value, so a command's renderer sees the one type it returns on
+// success and is never asked to recognise an Outcome.
+type Result struct {
+	Outcome
+	Value any
+}
+
+// Outcome is the part of a Result that says whether the command worked and, in words a
+// caller can show someone, why. It is what a program receives from a command that acts
+// rather than reports: closing a tunnel, changing a remote, reloading config.
+//
+// One shape serves all of them. Designing a payload per outcome would be work without a
+// reader. Reason is set either way, so a success says what it did rather than leaving
+// the caller to infer it from a bare true.
+type Outcome struct {
+	OK     bool   `json:"ok"`
+	Reason string `json:"reason"`
+}
+
+// Value is the Result of a command that reports: v is what it has to show, and it must
+// marshal to JSON.
+func Value(v any) Result {
+	return Result{Outcome: Outcome{OK: true}, Value: v}
+}
+
+// Failed is a convenience for the common "it did not work, here is why" case.
+func Failed(format string, args ...any) Result {
+	return Result{Outcome: Outcome{Reason: fmt.Sprintf(format, args...)}}
+}
+
+// Did is a convenience for the common "it worked, here is what happened" case.
+func Did(format string, args ...any) Result {
+	return Result{Outcome: Outcome{OK: true, Reason: fmt.Sprintf(format, args...)}}
+}
+
+// Nothing is the Result of a command with nothing to say: it did its work, and the
+// surface sends an empty, successful reply without rendering anything.
+func Nothing() Result {
+	return Result{Outcome: Outcome{OK: true}}
+}
+
+// Empty reports a Result with nothing to render: no Value and no Reason. The zero Result
+// counts, so a Run that returns Result{} says nothing rather than failing silently.
+func (r Result) Empty() bool {
+	return r.Value == nil && r.Reason == ""
+}
+
+// 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.
+func (r Result) Payload() any {
+	if r.Value != nil {
+		return r.Value
+	}
+	return r.Outcome
+}
+
+// Typed adapts a renderer for one result type to the TextFunc a Command declares. The
+// value Render passes is the command's own, see Result, so all that is left is to name
+// its type once here instead of asserting it at the top of every renderer. A renderer
+// paired with a command that returns something else fails here, saying what arrived.
+func Typed[T any](render func(T, io.Writer) error) TextFunc {
+	return func(v any, w io.Writer) error {
+		t, ok := v.(T)
+		if !ok {
+			return fmt.Errorf("expected %T, got %T", t, v)
+		}
+		return render(t, w)
+	}
+}

commands/render_test.go

@@ -0,0 +1,105 @@
+package commands
+
+import (
+	"bytes"
+	"io"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestRenderPrintsAnOutcomeWithoutText: a command that acts declares no Text. Its
+// outcome renders as the reason, the same for every command, and as {ok, reason} in
+// JSON, which is the shape these commands have always emitted.
+func TestRenderPrintsAnOutcomeWithoutText(t *testing.T) {
+	c := &Command{Name: "act"}
+
+	out := &bytes.Buffer{}
+	require.NoError(t, Render(c, Did("Closed"), FormatText, out))
+	assert.Equal(t, "Closed\n", out.String())
+
+	out.Reset()
+	require.NoError(t, Render(c, Failed("No vpn address was provided"), FormatText, out))
+	assert.Equal(t, "No vpn address was provided\n", out.String())
+
+	out.Reset()
+	require.NoError(t, Render(c, Failed("nope"), FormatJSON, out))
+	assert.JSONEq(t, `{"ok":false,"reason":"nope"}`, out.String())
+}
+
+// TestRenderHandsTextOnlyTheValue pins the guarantee Result exists for: a command's
+// renderer sees the type it returns on success and never the envelope or an outcome, so
+// a report command that refuses its arguments needs nothing in its Text to cope.
+func TestRenderHandsTextOnlyTheValue(t *testing.T) {
+	var got any
+	c := &Command{
+		Name: "report",
+		Text: func(v any, w io.Writer) error {
+			got = v
+			_, err := io.WriteString(w, "rendered\n")
+			return err
+		},
+	}
+
+	out := &bytes.Buffer{}
+	require.NoError(t, Render(c, Value("data"), FormatText, out))
+	assert.Equal(t, "data", got)
+	assert.Equal(t, "rendered\n", out.String())
+
+	got, out = nil, &bytes.Buffer{}
+	require.NoError(t, Render(c, Failed("refused"), FormatText, out))
+	assert.Nil(t, got, "Text must not be called for an outcome")
+	assert.Equal(t, "refused\n", out.String())
+
+	out.Reset()
+	require.NoError(t, Render(c, Value("data"), FormatJSON, out))
+	assert.JSONEq(t, `"data"`, out.String(), "JSON carries the value, never the envelope")
+}
+
+// TestRenderRefusesTextForAValueWithoutText: leaving Text nil is a statement that no
+// human form of the value exists yet, and asking for one is an error that says so.
+func TestRenderRefusesTextForAValueWithoutText(t *testing.T) {
+	out := &bytes.Buffer{}
+	err := Render(&Command{Name: "print-tunnel"}, Value(map[string]int{"index": 7}), FormatText, out)
+	require.ErrorIs(t, err, ErrNoTextRendering)
+	require.ErrorContains(t, err, "print-tunnel has no human readable output yet")
+	assert.Empty(t, out.String())
+}
+
+// TestTypedNamesTheTypeOnce: Typed replaces the assertion at the top of every renderer.
+// The right type reaches the renderer; the wrong one is reported, naming both.
+func TestTypedNamesTheTypeOnce(t *testing.T) {
+	type info struct{ Name string }
+	render := Typed(func(i info, w io.Writer) error {
+		_, err := io.WriteString(w, i.Name+"\n")
+		return err
+	})
+
+	out := &bytes.Buffer{}
+	require.NoError(t, render(info{Name: "tun0"}, out))
+	assert.Equal(t, "tun0\n", out.String())
+
+	err := render("not an info", out)
+	require.Error(t, err)
+	assert.ErrorContains(t, err, "expected commands.info, got string")
+}
+
+// TestResultShapes pins what each constructor means to a surface: Empty is the one that
+// renders nothing, Payload is what a program receives, and OK is set wherever a reason
+// or value says the command worked.
+func TestResultShapes(t *testing.T) {
+	assert.True(t, Nothing().Empty())
+	assert.True(t, Result{}.Empty(), "the zero Result says nothing rather than failing silently")
+	assert.False(t, Did("done").Empty())
+	assert.False(t, Failed("no").Empty())
+	assert.False(t, Value("x").Empty())
+
+	assert.True(t, Value("x").OK)
+	assert.True(t, Did("done").OK)
+	assert.False(t, Failed("no").OK)
+
+	assert.Equal(t, "x", Value("x").Payload())
+	assert.Equal(t, Outcome{OK: true, Reason: "done"}, Did("done").Payload())
+	assert.Equal(t, Outcome{Reason: "no"}, Failed("no").Payload())
+}

commands/request.go

@@ -2,18 +2,20 @@ package commands
 
 import (
 	"flag"
-	"fmt"
 	"maps"
 	"slices"
 )
 
 // Request is one command to run, as every surface hands it to Invoke: the command, its
-// options by name in the form their flags parse, and its operands.
+// options by name in the form their flags parse, its operands, and the rendering the
+// line asked for, if any.
 //
-// It is where the surfaces meet. An ssh line is parsed against the command's own FlagSet
-// and becomes one of these through ParseLine; nebula ctl parses its line the same way
-// against the schema and sends the pieces over the wire, where the control socket turns
-// them back into one. From here on there is one path to the command.
+// 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.
 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.
@@ -25,16 +27,28 @@ type Request struct {
 
 	// Args carries the operands: whatever the line did not consume as a flag.
 	Args []string
+
+	// Format is the rendering the line asked for with -json or -pretty, or FormatUnset
+	// when it expressed no preference. It is ParseLine's reading of the line for the
+	// client that sends the Request; Invoke does not read it, since the surface that
+	// runs a Request renders with the format it agreed with its caller.
+	Format Format
 }
 
-// ParseLine reads a FlagSet that Parse has filled, and the operands it returned, into a
-// Request. Command is left for the caller to set: it resolved the command before it
-// could build the FlagSet. Every flag the line set travels by name in the form it parses
-// from, which is what Invoke applies.
+// ParseLine reads a FlagSet that ParseArgs has filled, and the operands it returned,
+// into a Request to send over the wire. Command is left for the caller to set.
+//
+// Every flag the line set other than the presentation flags travels by name, as its
+// flag's String, which is what the daemon's Set parses. That round trip is exact for
+// the bool and string flags a client builds from the schema; it is the client's to make,
+// and the ssh path does not make it, since it runs the FlagSet it parsed.
 func ParseLine(fl *flag.FlagSet, operands []string) Request {
-	req := Request{Args: operands}
+	req := Request{Args: operands, Format: presentationFormat(fl)}
 
 	fl.Visit(func(f *flag.Flag) {
+		if isPresentationFlag(f.Name) {
+			return
+		}
 		if req.Params == nil {
 			req.Params = map[string]string{}
 		}
@@ -44,41 +58,65 @@ func ParseLine(fl *flag.FlagSet, operands []string) Request {
 	return req
 }
 
-// Invoke runs one Request, with w as the command's output.
+// presentationFormat is the one reading of -json and -pretty from a parsed line, so a
+// line means the same on every surface: only a flag set to true asks for JSON, since one
+// set to false expresses no preference, and -pretty implies -json. FormatUnset means the
+// line expressed no preference and the surface's own default applies.
+func presentationFormat(fl *flag.FlagSet) Format {
+	var asJSON, pretty bool
+	fl.Visit(func(f *flag.Flag) {
+		switch f.Name {
+		case FlagJSON:
+			asJSON = flagIsTrue(f)
+		case FlagPretty:
+			pretty = flagIsTrue(f)
+		}
+	})
+
+	switch {
+	case pretty:
+		return FormatPrettyJSON
+	case asJSON:
+		return FormatJSON
+	default:
+		return FormatUnset
+	}
+}
+
+// flagIsTrue reports whether a bool flag was set to true. The flag package's bool value
+// is a flag.Getter whose Get returns the bool, so this never falls back to parsing text.
+func flagIsTrue(f *flag.Flag) bool {
+	g, ok := f.Value.(flag.Getter)
+	if !ok {
+		return false
+	}
+	b, _ := g.Get().(bool)
+	return b
+}
+
+// Invoke runs one Request and returns the command's Result, unrendered.
 //
 // The parameters are applied by flag name, in name order: a request from the wire is a
 // JSON object with no order of its own, and which of two bad parameters is reported must
 // not change from one request to the next. An unknown parameter is an error rather than
 // something quietly ignored, because a caller that misspells one deserves to hear about
-// it instead of getting a default. A bad parameter means the command is never called,
-// and the FlagSet is fresh for this call, so nothing set before it is seen by anyone.
-func Invoke(req Request, w StringWriter) error {
+// it instead of getting a default. A bad parameter means Run is never called, and the
+// FlagSet is fresh for this call, so nothing set before it is seen by anyone.
+//
+// Nothing here writes anywhere and nothing here renders. What a surface does with the
+// Result, marshal its Payload or run the command's own text rendering through Render,
+// is the surface's decision, and keeping that out of here is what lets a caller tell a
+// refusal from a success after the text has been produced. The presentation flags are
+// not parameters: a Request says how it wants rendering in Format, and sending them as
+// parameters is refused rather than read as a preference.
+func Invoke(req Request) (Result, error) {
 	c := req.Command
 	names := slices.Sorted(maps.Keys(req.Params))
 
-	var fs any
-	if c.Flags != nil {
-		fl, box := c.Flags()
-		fs = box
-
-		if fl != nil {
-			fl.SetOutput(w.Err())
-
-			for _, k := range names {
-				if fl.Lookup(k) == nil {
-					return fmt.Errorf("%s has no parameter %q", c.Name, k)
-				}
-
-				if err := fl.Set(k, req.Params[k]); err != nil {
-					return fmt.Errorf("%s: %w", k, err)
-				}
-			}
-		} else if len(names) > 0 {
-			return fmt.Errorf("%s takes no parameters", c.Name)
-		}
-	} else if len(names) > 0 {
-		return fmt.Errorf("%s takes no parameters", c.Name)
+	fl, fs, own := commandFlags(c)
+	if err := setParams(fl, string(c.Name), names, req.Params, own); err != nil {
+		return Result{}, err
 	}
 
-	return c.Callback(fs, req.Args, w)
+	return c.Run(fs, req.Args)
 }

commands/request_test.go

@@ -1,53 +1,71 @@
 package commands
 
 import (
-	"bytes"
 	"flag"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-// TestParseLineCarriesWhatWasSetInParseableForm: only the flags the line set travel,
-// each as the string its flag parses from, so Invoke can apply them to a fresh FlagSet
-// and get the same values back. Operands come through untouched.
-func TestParseLineCarriesWhatWasSetInParseableForm(t *testing.T) {
-	type seen struct {
-		Verbose bool
-		Label   string
-	}
-	var got []seen
+func lineFlags() *flag.FlagSet {
+	fl := flag.NewFlagSet("set", flag.ContinueOnError)
+	fl.Bool("verbose", false, "")
+	fl.Duration("wait", 0, "")
+	fl.String("label", "none", "")
+	fl.Bool(FlagJSON, false, "")
+	fl.Bool(FlagPretty, false, "")
+	return fl
+}
 
-	r := New()
-	r.Register(&Command{
-		Name: "set",
-		Flags: func() (*flag.FlagSet, any) {
-			fl := flag.NewFlagSet("", flag.ContinueOnError)
-			s := seen{}
-			fl.BoolVar(&s.Verbose, "verbose", false, "say more")
-			fl.StringVar(&s.Label, "label", "none", "a label")
-			return fl, &s
-		},
-		Callback: func(fs any, a []string, w StringWriter) error {
-			got = append(got, *fs.(*seen))
-			return w.WriteLine("ran")
-		},
-	})
+// TestParseLineReadsThePresentationFlagsOnce: presentationFormat is the one reading of
+// -json and -pretty, for every surface, and ParseLine carries its answer. A flag set to
+// false expresses no preference, and neither flag ever travels as a parameter.
+func TestParseLineReadsThePresentationFlagsOnce(t *testing.T) {
+	for _, tc := range []struct {
+		args []string
+		want Format
+	}{
+		{nil, FormatUnset},
+		{[]string{"-json=false"}, FormatUnset},
+		{[]string{"-pretty=false"}, FormatUnset},
+		{[]string{"-json"}, FormatJSON},
+		{[]string{"-pretty"}, FormatPrettyJSON},
+		{[]string{"-pretty", "-json=false"}, FormatPrettyJSON},
+	} {
+		fl := lineFlags()
+		operands, err := ParseArgs(fl, tc.args)
+		require.NoError(t, err)
+
+		req := ParseLine(fl, operands)
+		assert.Equal(t, tc.want, req.Format, "%v", tc.args)
+		assert.Nil(t, req.Params, "%v: presentation flags are not parameters", tc.args)
+	}
+}
 
-	fl, _ := r.Get("set").Flags()
-	require.NoError(t, fl.Parse([]string{"-verbose", "x", "y"}))
+// TestParseLineCarriesWhatWasSetInParseableForm: only the flags the line set travel,
+// each as the string its flag parses from, so the daemon's Invoke can apply them to a
+// fresh FlagSet and get the same values back. Operands come through untouched, those
+// after "--" included.
+func TestParseLineCarriesWhatWasSetInParseableForm(t *testing.T) {
+	fl := lineFlags()
+	operands, err := ParseArgs(fl, []string{"-verbose", "-wait", "2s", "x", "--", "-1"})
+	require.NoError(t, err)
 
-	req := ParseLine(fl, fl.Args())
-	assert.Equal(t, map[string]string{"verbose": "true"}, req.Params, "label was left alone and must be absent")
-	assert.Equal(t, []string{"x", "y"}, req.Args)
+	req := ParseLine(fl, operands)
+	assert.Equal(t, map[string]string{"verbose": "true", "wait": "2s"}, req.Params, "label was left alone and must be absent")
+	assert.Equal(t, []string{"x", "-1"}, req.Args)
+	assert.Equal(t, FormatUnset, req.Format)
 
-	// The round trip: what ParseLine produced is what Invoke applies.
+	// The round trip a client makes: what ParseLine produced is what Invoke applies.
+	r, seen := paramRegistry(t)
 	req.Command = r.Get("set")
-	out := &bytes.Buffer{}
-	require.NoError(t, Invoke(req, NewStringWriter(out)))
-	assert.Equal(t, "ran\n", out.String())
-	assert.Equal(t, []seen{{Verbose: true, Label: "none"}}, got)
+	_, err = Invoke(req)
+	require.NoError(t, err)
+	require.Len(t, *seen, 1)
+	assert.Equal(t, paramsSeen{Verbose: true, Wait: 2 * time.Second}, (*seen)[0])
 }
 
 // TestResolveTellsUnknownFromWithheld: the two cases read differently to an operator,
@@ -65,3 +83,33 @@ func TestResolveTellsUnknownFromWithheld(t *testing.T) {
 	_, err = r.Resolve("no-such-command")
 	require.ErrorContains(t, err, `unknown command "no-such-command"`)
 }
+
+// TestLineRunsTheFlagSetItParsed: the ssh path hands Run the FlagSet the line was parsed
+// into, with no trip through a Request. A flag.Func has no String worth re-parsing, so a
+// round trip would hand Run an empty value and no error; the line's value must arrive.
+func TestLineRunsTheFlagSetItParsed(t *testing.T) {
+	var seen []string
+	r := New()
+	r.Register(&Command{
+		Name: "mode",
+		Flags: func() (*flag.FlagSet, any) {
+			fl := flag.NewFlagSet("", flag.ContinueOnError)
+			s := new(string)
+			fl.Func("mode", "the mode", func(v string) error {
+				*s = "set:" + v
+				return nil
+			})
+			return fl, s
+		},
+		Run: func(fs any, a []string) (Result, error) {
+			seen = append(seen, *fs.(*string))
+			return Value("ok"), nil
+		},
+	})
+
+	var out, errw strings.Builder
+	r.Dispatch("mode -json -mode fast", Output{Out: &out, Err: &errw})
+	require.Equal(t, []string{"set:fast"}, seen)
+	assert.Empty(t, errw.String())
+	assert.Equal(t, "\"ok\"\n", out.String(), "-json was read from the same FlagSet")
+}

commands/writer.go

@@ -2,54 +2,22 @@ package commands
 
 import "io"
 
-// StringWriter is how a command talks back to whoever invoked it.
+// Output is where a command line surface's text goes.
 //
-// Out and Err are distinct so a command's primary output stays machine readable on
-// surfaces that separate the two (the local control socket maps them to different
-// frames). The ssh surface points both at the same channel, which is why it sees a
-// single merged stream exactly as it always has.
-type StringWriter interface {
-	WriteLine(string) error
-	Write(string) error
-	WriteBytes([]byte) error
-	GetWriter() io.Writer
-	Err() io.Writer
-}
-
-type stringWriter struct {
-	w io.Writer
-	e io.Writer
-}
-
-// NewStringWriter returns a StringWriter that merges output and errors onto w.
-func NewStringWriter(w io.Writer) StringWriter {
-	return &stringWriter{w: w, e: w}
-}
-
-// NewSplitStringWriter returns a StringWriter that sends command output to out and
-// usage/error text to err.
-func NewSplitStringWriter(out, err io.Writer) StringWriter {
-	return &stringWriter{w: out, e: err}
-}
-
-func (w *stringWriter) WriteLine(s string) error {
-	return w.Write(s + "\n")
-}
-
-func (w *stringWriter) Write(s string) error {
-	_, err := w.w.Write([]byte(s))
-	return err
-}
-
-func (w *stringWriter) WriteBytes(b []byte) error {
-	_, err := w.w.Write(b)
-	return err
-}
-
-func (w *stringWriter) GetWriter() io.Writer {
-	return w.w
-}
-
-func (w *stringWriter) Err() io.Writer {
-	return w.e
+// 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
+// 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 {
+	// Out receives what a command produces.
+	Out io.Writer
+
+	// Err receives usage text, refusals and errors.
+	Err io.Writer
+}
+
+// MergedOutput sends output and errors to the same writer.
+func MergedOutput(w io.Writer) Output {
+	return Output{Out: w, Err: w}
 }

commands_test.go

@@ -2,6 +2,7 @@ package nebula
 
 import (
 	"bytes"
+	"encoding/json"
 	"io"
 	"net/netip"
 	"testing"
@@ -13,19 +14,20 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TestLogFormatRefusalIsShown: a format the logger does not know is reported to the user
-// as a failure, rather than returned bare, which over ssh was silence.
+// TestLogFormatRefusalIsShown: a format the logger does not know is a refusal the user
+// sees, rather than a bare error, which over ssh was silence.
 func TestLogFormatRefusalIsShown(t *testing.T) {
 	l := logging.NewLogger(io.Discard)
-	out := &bytes.Buffer{}
-	w := commands.NewStringWriter(out)
 
-	var f *commands.Failure
-	require.ErrorAs(t, logFormatCommand(l, nil, []string{"bogus"}, w), &f)
-	assert.Equal(t, "Unable to set log format: unknown log format `bogus`. possible formats: [text json]\n", out.String())
+	res, err := runLogFormat(l, []string{"bogus"})
+	require.NoError(t, err)
+	assert.False(t, res.OK)
+	assert.Equal(t, "Unable to set log format: unknown log format `bogus`. possible formats: [text json]", res.Reason)
 
-	out.Reset()
-	require.NoError(t, logFormatCommand(l, nil, []string{"JSON"}, w))
+	res, err = runLogFormat(l, []string{"JSON"})
+	require.NoError(t, err)
+	out := &bytes.Buffer{}
+	require.NoError(t, commands.Typed(textLogFormat)(res.Value, out))
 	assert.Equal(t, "Log format is: json\n", out.String())
 }
 
@@ -41,9 +43,12 @@ func TestPrintRelaysReportsErrorsAsText(t *testing.T) {
 	hm.Relays[1] = &HostInfo{vpnAddrs: []netip.Addr{relay}}
 	ifce := &Interface{hostMap: hm}
 
-	out := &bytes.Buffer{}
-	require.NoError(t, printRelaysCommand(ifce, &printTunnelFlags{}, nil, commands.NewStringWriter(out)))
-	assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Error":"could not find hostinfo","Type":"","State":"","PeerAddr":"","LocalIndex":0,"RemoteIndex":0,"RelayedThrough":null}]}]}`, out.String())
+	marshal := func() string {
+		b, err := json.Marshal(runPrintRelays(ifce))
+		require.NoError(t, err)
+		return string(b)
+	}
+	assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Error":"could not find hostinfo","Type":"","State":"","PeerAddr":"","LocalIndex":0,"RemoteIndex":0,"RelayedThrough":null}]}]}`, marshal())
 
 	// The same relay host, found, relaying for a peer whose index matches: no Error key.
 	hi := hm.Relays[1]
@@ -53,15 +58,10 @@ func TestPrintRelaysReportsErrorsAsText(t *testing.T) {
 		relayForByIdx:  map[uint32]*Relay{r.LocalIndex: r},
 	}
 	hm.Hosts[relay] = hi
-
-	out.Reset()
-	require.NoError(t, printRelaysCommand(ifce, &printTunnelFlags{}, nil, commands.NewStringWriter(out)))
-	assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Type":"forwarding","State":"established","PeerAddr":"10.0.0.2","LocalIndex":1,"RemoteIndex":2,"RelayedThrough":null}]}]}`, out.String())
+	assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Type":"forwarding","State":"established","PeerAddr":"10.0.0.2","LocalIndex":1,"RemoteIndex":2,"RelayedThrough":null}]}]}`, marshal())
 
 	// And an index mismatch names it.
 	hm.Relays[7] = hi
 	delete(hm.Relays, 1)
-	out.Reset()
-	require.NoError(t, printRelaysCommand(ifce, &printTunnelFlags{}, nil, commands.NewStringWriter(out)))
-	assert.Contains(t, out.String(), `"Error":"hostmap LocalIndex '7' does not match RelayState LocalIndex"`)
+	assert.Contains(t, marshal(), `"Error":"hostmap LocalIndex '7' does not match RelayState LocalIndex"`)
 }

e2e/commands_output_test.go

@@ -69,12 +69,15 @@ func TestCommandOutput(t *testing.T) {
 	// help on the clone lists and describes. There is no session to end here.
 	reg := control.Commands().Clone()
 	reg.Register(sshd.LogoutCommand(func() {}))
+	// Dispatch with ssh's default-format policy, so the no-flag column records what an
+	// operator at an ssh prompt sees.
+	reg.DefaultFormat = sshd.DefaultFormat
 
 	// run dispatches as the ssh exec path does and returns the output and the error, which
 	// over ssh is exit status 1.
 	run := func(args ...string) (string, error) {
 		buf := &bytes.Buffer{}
-		err := reg.DispatchArgs(args, commands.NewStringWriter(buf))
+		err := reg.DispatchArgs(args, commands.MergedOutput(buf))
 		return buf.String(), err
 	}
 
@@ -94,6 +97,8 @@ func TestCommandOutput(t *testing.T) {
 		{"print-cert", nil},
 		{"print-cert", []string{"-raw"}},
 		{"print-cert", []string{"10.222.0.9"}},
+		// A flag after the operand is still a flag, on both surfaces.
+		{"print-cert", []string{"10.222.0.9", "-json"}},
 		{"print-cert", []string{"10.222.0.250"}},
 		{"print-cert", []string{"not-an-address"}},
 		{"print-tunnel", nil},

localapi/client.go

@@ -78,8 +78,10 @@ func (c *Client) Describe() ([]commands.CommandSchema, error) {
 
 // Invoke runs one command and returns what the daemon said. params are the command's
 // options by name, as the strings a command line parsed them from; a program speaking
-// the protocol directly may send JSON's own types instead, see Request.Params.
-func (c *Client) Invoke(command string, params map[string]string, args []string) (*Response, error) {
+// the protocol directly may send JSON's own types instead, see Request.Params. An empty
+// format asks for JSON, which is the API's default; "text" asks the daemon to apply the
+// command's own human rendering.
+func (c *Client) Invoke(command string, params map[string]string, args []string, format string) (*Response, error) {
 	var wire map[string]any
 	if len(params) > 0 {
 		wire = make(map[string]any, len(params))
@@ -93,6 +95,7 @@ func (c *Client) Invoke(command string, params map[string]string, args []string)
 		Command: command,
 		Params:  wire,
 		Args:    args,
+		Format:  format,
 	})
 }
 
@@ -161,14 +164,16 @@ func readResponse(conn net.Conn) (*Response, error) {
 }
 
 // Pretty renders a JSON Result indented for a human reader, reporting false when the
-// result is not a JSON value and there is nothing to indent.
+// result is empty or is not JSON and there is nothing to indent. A value that is a JSON
+// string has nothing to indent either, and comes back as it is, quotes and all: the
+// caller asked for JSON and that is what the value is.
 //
 // The bytes are re-indented rather than decoded and re-encoded: decoding into `any`
 // sorts object keys and routes every number through float64, so the layout would
 // differ from what the daemon wrote and from the ssh -pretty path.
 func Pretty(raw json.RawMessage) (string, bool) {
 	trimmed := bytes.TrimSpace(raw)
-	if len(trimmed) == 0 || trimmed[0] == '"' {
+	if len(trimmed) == 0 {
 		return "", false
 	}
 
@@ -180,16 +185,18 @@ func Pretty(raw json.RawMessage) (string, bool) {
 	return buf.String(), true
 }
 
-// Text renders a Result for a human reader. A JSON value is written through unchanged so
-// the command's own formatting survives; a JSON string is unquoted back into the text the
-// command actually wrote.
+// Text renders the Result of a request that asked for text. The daemon carries a text
+// rendering as a JSON string, which is unquoted back into the bytes the command wrote.
+// It is the caller's job to know that text was asked for: a JSON result whose value is
+// a string looks the same on the wire and must not be unquoted, so the choice is made
+// from the request's format, never from the bytes.
 func Text(raw json.RawMessage) string {
 	if len(raw) == 0 {
 		return ""
 	}
 
 	// Only an actual JSON string is unquoted. Unmarshalling into a string also accepts
-	// null, which would turn a command's `null` answer into no output at all.
+	// null, which would turn a `null` answer into no output at all.
 	if raw[0] == '"' {
 		var s string
 		if err := json.Unmarshal(raw, &s); err == nil {

localapi/client_test.go

@@ -29,13 +29,16 @@ func TestPrettyPreservesKeyOrderAndPrecision(t *testing.T) {
 }`, out)
 }
 
-func TestPrettyLeavesTextAlone(t *testing.T) {
-	// A JSON string is a command's plain text output; it is rendered by Text, not indented.
-	_, ok := Pretty(json.RawMessage(`"Log level is: info"`))
-	assert.False(t, ok)
+func TestPrettyKeepsAStringValueQuoted(t *testing.T) {
+	// A value that is a JSON string is still JSON to whoever asked for it: it has nothing
+	// to indent and comes back as it is, never unquoted.
+	out, ok := Pretty(json.RawMessage(`"some string"`))
+	assert.True(t, ok)
+	assert.Equal(t, `"some string"`, out)
 
-	_, ok = Pretty(json.RawMessage(`  "padded"`))
-	assert.False(t, ok)
+	out, ok = Pretty(json.RawMessage(`  "padded"`))
+	assert.True(t, ok)
+	assert.Equal(t, `"padded"`, out)
 }
 
 func TestPrettyRejectsEmptyAndInvalid(t *testing.T) {

localapi/protocol.go

@@ -48,13 +48,21 @@ type Request struct {
 	// Params carries the command's options by name, with JSON's own types, true rather
 	// than "true", 3 rather than "3", or as the strings a command line parsed them from,
 	// which is what nebula ctl sends. Unknown names are an error, so a typo is reported
-	// instead of ignored.
+	// instead of ignored. The schema lists json and pretty on every command because the
+	// command lines take them; here they are refused, since Format is how a request
+	// chooses its rendering.
 	Params map[string]any `json:"params,omitempty"`
 
 	// Args carries the command's operands, the values that are not options: the vpn
 	// address `print-tunnel` prints, for instance. They stay positional because the
 	// commands do not name them.
 	Args []string `json:"args,omitempty"`
+
+	// Format asks for a rendering other than the default. "" and "json" return the
+	// command's result as a JSON value, which is what a program wants. "text" asks the
+	// daemon to run the command's own human rendering and return it as a string, which is
+	// how `nebula ctl` shows an operator the same output the ssh server would.
+	Format string `json:"format,omitempty"`
 }
 
 // Response is the single JSON object a server sends back.
@@ -75,29 +83,13 @@ type Response struct {
 	// signal a caller needs: there is no separate status field, because a command that
 	// failed always says why and one that succeeded never does. A command line client
 	// turns its presence into a non-zero exit.
+	//
+	// A command that ran and refused sets Error too, alongside its Result when the
+	// caller asked for JSON. That is how a refusal and a request that never ran are
+	// told apart: the first has both, the second has Error alone.
 	Error string `json:"error,omitempty"`
 }
 
-// jsonOrString renders b as a JSON value when it is one, and as a JSON string when it is
-// not. A command writes bytes; when those bytes are JSON a caller reads them directly
-// rather than parsing a string that happens to contain JSON.
-func jsonOrString(b []byte) json.RawMessage {
-	if len(b) == 0 {
-		return nil
-	}
-
-	if json.Valid(b) {
-		return json.RawMessage(b)
-	}
-
-	quoted, err := json.Marshal(string(b))
-	if err != nil {
-		return nil
-	}
-
-	return quoted
-}
-
 // frameError is a frame that arrived and was not one: oversized, or not the JSON object
 // the protocol defines. It is told from a failed read so the client can say the daemon
 // answered with something it could not read, rather than that it could not be reached.

localapi/server.go

@@ -295,7 +295,10 @@ func (s *Server) accept(ln net.Listener, sem chan struct{}) {
 func (s *Server) handle(conn net.Conn) {
 	defer conn.Close()
 
-	var args []string
+	var (
+		args    []string
+		replied bool
+	)
 	defer func() {
 		if r := recover(); r != nil {
 			s.l.Error("Recovered from panic serving a local control command",
@@ -303,7 +306,11 @@ func (s *Server) handle(conn net.Conn) {
 				"args", args,
 				"stack", string(debug.Stack()),
 			)
-			s.reply(conn, &Response{Version: ProtocolVersion, Error: "command failed"})
+			// A panic in a command's AfterReply arrives here after the answer has gone
+			// out; the client has its reply, so it is not sent a second one.
+			if !replied {
+				s.reply(conn, &Response{Version: ProtocolVersion, Error: "command failed"})
+			}
 		}
 	}()
 
@@ -340,17 +347,25 @@ func (s *Server) handle(conn net.Conn) {
 	_ = conn.SetReadDeadline(time.Time{})
 	_ = conn.SetWriteDeadline(time.Now().Add(writeTimeout))
 
-	s.reply(conn, s.serve(req))
+	res, after := s.serve(req)
+	s.reply(conn, res)
+	replied = true
+
+	// A command that would cut off its own reply acts only now, once the answer is out.
+	if after != nil {
+		after()
+	}
 }
 
-// serve runs one request and describes the outcome.
+// serve runs one request and describes the outcome. after, when not nil, is the
+// command's AfterReply, which the caller runs once the response has been written.
 //
 // Nothing bounds a command's run; a context would have to reach every Run. What is
 // bounded is the silence: a command still running at slowAfter is named in the log, with
 // the slot it is holding, so a socket that has gone "busy" can be traced to the commands
 // that wedged it. The timer is stopped however the command ends, a panic included, so
 // the line is only ever written about a command that is still running.
-func (s *Server) serve(req *Request) *Response {
+func (s *Server) serve(req *Request) (res *Response, after func()) {
 	slow := time.AfterFunc(s.slowAfter, func() {
 		s.l.Warn("Local control command is still running and holding a connection slot",
 			"command", req.Command,
@@ -361,60 +376,119 @@ func (s *Server) serve(req *Request) *Response {
 	})
 	defer slow.Stop()
 
-	res := &Response{Version: ProtocolVersion}
+	res = &Response{Version: ProtocolVersion}
+	reg := s.registry()
 
 	if req.Describe {
-		schema, err := json.Marshal(s.registry().Schema())
+		schema, err := json.Marshal(reg.Schema())
 		if err != nil {
 			res.Error = err.Error()
-			return res
+			return res, nil
 		}
 
 		res.Result = schema
-		return res
+		return res, nil
 	}
 
 	if req.Command == "" {
 		res.Error = "no command given"
-		return res
+		return res, nil
 	}
 
 	params, err := stringParams(req.Params)
 	if err != nil {
 		res.Error = err.Error()
-		return res
+		return res, nil
 	}
 
-	cmd, err := s.registry().Resolve(commands.Name(req.Command))
+	format, err := parseFormat(req.Format)
 	if err != nil {
-		s.l.Debug("Local control command refused", "command", req.Command, "error", err)
 		res.Error = err.Error()
-		return res
+		return res, nil
 	}
 
-	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
+	cmd, err := reg.Resolve(commands.Name(req.Command))
+	if err != nil {
+		s.l.Debug("Local control command refused", "command", req.Command, "error", err)
+		res.Error = err.Error()
+		return res, nil
+	}
 
-	// A command that failed may have explained itself on its error stream; if it did
-	// not, the returned error is the explanation.
-	if err := commands.Invoke(commands.Request{Command: cmd, Params: params, Args: req.Args}, commands.NewSplitStringWriter(out, errOut)); err != nil {
+	r, err := commands.Invoke(commands.Request{Command: cmd, Params: params, Args: req.Args})
+	if err != nil {
 		s.l.Debug("Local control command failed", "command", req.Command, "error", err)
+		res.Error = err.Error()
+		return res, nil
+	}
 
-		// Whatever the command wrote is the better explanation; the returned error is the
-		// fallback for one that failed without saying so.
-		if errOut.Len() == 0 {
-			res.Error = err.Error()
+	after = cmd.AfterReply
+
+	// A command with nothing to say gets an empty, successful reply.
+	if r.Empty() {
+		return res, after
+	}
+
+	return render(res, cmd, r, format), after
+}
+
+// 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.
+func render(res *Response, cmd *commands.Command, r commands.Result, format commands.Format) *Response {
+	buf := &bytes.Buffer{}
+	if err := commands.Render(cmd, r, format, buf); err != nil {
+		res.Error = err.Error()
+		return res
+	}
+
+	if format != commands.FormatText {
+		// The caller is a program. Carry the payload as JSON rather than as a string that
+		// happens to contain some; the newline Render ends a line with is not the value's.
+		res.Result = bytes.TrimSuffix(buf.Bytes(), []byte("\n"))
+
+		// A command that ran and refused is a failure to whoever asked, in either format.
+		// The structured outcome still travels in Result for a program to read; Error
+		// carries the reason so the one rule, Error set means failed, holds here too and
+		// a client never has to guess from the payload.
+		if !r.OK {
+			res.Error = r.Reason
 		}
+
+		return res
 	}
 
-	if errOut.Len() > 0 {
-		res.Error = errOut.String()
+	// A command that ran and refused reads as an error to whoever asked for text: they
+	// are a person or a script, and both expect a refusal on the error stream.
+	if !r.OK {
+		res.Error = buf.String()
+		return res
 	}
 
-	// The command wrote bytes; carry them as a JSON value when they are one.
-	res.Result = jsonOrString(out.Bytes())
+	quoted, err := json.Marshal(buf.String())
+	if err != nil {
+		res.Error = err.Error()
+		return res
+	}
+
+	res.Result = quoted
 	return res
 }
 
+// parseFormat turns the request's format into the one the registry understands. An
+// unknown value is an error rather than a silent default, so a client that asks for
+// something this daemon cannot do hears about it.
+func parseFormat(s string) (commands.Format, error) {
+	switch s {
+	case "", "json":
+		return commands.FormatJSON, nil
+	case "text":
+		return commands.FormatText, nil
+	default:
+		return commands.FormatJSON, fmt.Errorf("unknown format %q, expected \"json\" or \"text\"", s)
+	}
+}
+
 // reply writes one response frame.
 func (s *Server) reply(conn net.Conn, res *Response) {
 	_ = conn.SetWriteDeadline(time.Now().Add(writeTimeout))

localapi/server_test.go

@@ -9,10 +9,12 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"io"
 	"math"
 	"net"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -57,8 +59,8 @@ func echoCommand() *commands.Command {
 	return &commands.Command{
 		Name:             "echo",
 		ShortDescription: "echoes its arguments",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return w.Write(fmt.Sprintf("%v", a))
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value(fmt.Sprintf("%v", a)), nil
 		},
 	}
 }
@@ -70,14 +72,14 @@ func basicRegistry() *commands.Registry {
 		Name:             "withheld",
 		ShortDescription: "a command this surface withholds",
 		ForProfiling:     true,
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return w.Write("mutated")
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value("mutated"), nil
 		},
 	})
 	reg.Register(&commands.Command{
 		Name:             "boom",
 		ShortDescription: "panics",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
+		Run: func(fs any, a []string) (commands.Result, error) {
 			panic("this command is broken")
 		},
 	})
@@ -89,7 +91,7 @@ func basicRegistry() *commands.Registry {
 func run(t *testing.T, path, command string, args ...string) (string, string) {
 	t.Helper()
 
-	res, err := (&Client{Path: path}).Invoke(command, nil, args)
+	res, err := (&Client{Path: path}).Invoke(command, nil, args, "")
 	require.NoError(t, err)
 	return Text(res.Result), res.Error
 }
@@ -353,9 +355,9 @@ func TestConcurrentClientsAreBounded(t *testing.T) {
 	reg.Register(&commands.Command{
 		Name:             "block",
 		ShortDescription: "blocks until released",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
+		Run: func(fs any, a []string) (commands.Result, error) {
 			<-release
-			return w.Write("done")
+			return commands.Value("done"), nil
 		},
 	})
 
@@ -370,14 +372,14 @@ func TestConcurrentClientsAreBounded(t *testing.T) {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			_, _ = (&Client{Path: path}).Invoke("block", nil, nil)
+			_, _ = (&Client{Path: path}).Invoke("block", nil, nil, "")
 		}()
 	}
 
 	// Give the two blockers time to occupy the slots.
 	time.Sleep(200 * time.Millisecond)
 
-	res, err := (&Client{Path: path}).Invoke("block", nil, nil)
+	res, err := (&Client{Path: path}).Invoke("block", nil, nil, "")
 	require.NoError(t, err)
 	assert.Contains(t, res.Error, "busy")
 
@@ -385,6 +387,99 @@ func TestConcurrentClientsAreBounded(t *testing.T) {
 	wg.Wait()
 }
 
+// TestSlowCommandIsNamedInTheLog: a command that does not return holds its slot for
+// good, and enough of them leave the socket answering only "busy". Both are logged, the
+// command by name once it has run past slowAfter and each refused connection as it
+// happens, so a busy socket can be traced to what wedged it.
+func TestSlowCommandIsNamedInTheLog(t *testing.T) {
+	reg := commands.New()
+	release := make(chan struct{})
+	reg.Register(&commands.Command{
+		Name:             "block",
+		ShortDescription: "blocks until released",
+		Run: func(fs any, a []string) (commands.Result, error) {
+			<-release
+			return commands.Value("done"), nil
+		},
+	})
+
+	var logs lockedBuffer
+	s := NewServer(test.NewLoggerWithOutput(&logs), reg)
+	s.maxConns = 1
+	s.slowAfter = 20 * time.Millisecond
+	path := testSocket(t)
+	require.NoError(t, s.Rebind(context.Background(), path))
+	defer s.Stop()
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		_, _ = (&Client{Path: path}).Invoke("block", nil, []string{"x"}, "")
+	}()
+
+	require.Eventually(t, func() bool {
+		return strings.Contains(logs.String(), "still running")
+	}, 2*time.Second, 10*time.Millisecond)
+	assert.Contains(t, logs.String(), "command=block")
+	assert.Contains(t, logs.String(), "args=[x]")
+
+	res, err := (&Client{Path: path}).Invoke("block", nil, nil, "")
+	require.NoError(t, err)
+	assert.Contains(t, res.Error, "busy")
+	assert.Contains(t, logs.String(), "at capacity")
+
+	close(release)
+	wg.Wait()
+}
+
+// TestPanickedCommandIsNotReportedAsStillRunning: the slow command line is about a slot
+// that is still held. A command that panicked has given its slot back, and the client
+// its answer, so no line must claim otherwise after the fact.
+func TestPanickedCommandIsNotReportedAsStillRunning(t *testing.T) {
+	reg := commands.New()
+	reg.Register(&commands.Command{
+		Name:             "boom",
+		ShortDescription: "panics",
+		Run: func(fs any, a []string) (commands.Result, error) {
+			panic("this command is broken")
+		},
+	})
+
+	var logs lockedBuffer
+	s := NewServer(test.NewLoggerWithOutput(&logs), reg)
+	s.slowAfter = 20 * time.Millisecond
+	path := testSocket(t)
+	require.NoError(t, s.Rebind(context.Background(), path))
+	defer s.Stop()
+
+	res, err := (&Client{Path: path}).Invoke("boom", nil, nil, "")
+	require.NoError(t, err)
+	assert.Equal(t, "command failed", res.Error)
+
+	time.Sleep(100 * time.Millisecond)
+	assert.Contains(t, logs.String(), "Recovered from panic")
+	assert.NotContains(t, logs.String(), "still running")
+}
+
+// lockedBuffer is a strings.Builder the server's goroutines and the test can share.
+type lockedBuffer struct {
+	mu sync.Mutex
+	b  strings.Builder
+}
+
+func (l *lockedBuffer) Write(p []byte) (int, error) {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+	return l.b.Write(p)
+}
+
+func (l *lockedBuffer) String() string {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+	return l.b.String()
+}
+
 // flakyListener fails its first few Accepts the way a process that is out of file
 // descriptors does, then behaves.
 type flakyListener struct {
@@ -503,12 +598,12 @@ func TestNamedParametersDriveFlags(t *testing.T) {
 			fl.StringVar(&s.Label, "label", "none", "a label")
 			return fl, &s
 		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
+		Run: func(fs any, a []string) (commands.Result, error) {
 			s := fs.(*struct {
 				Verbose bool
 				Label   string
 			})
-			return w.Write(fmt.Sprintf("verbose=%v label=%s args=%v", s.Verbose, s.Label, a))
+			return commands.Value(fmt.Sprintf("verbose=%v label=%s args=%v", s.Verbose, s.Label, a)), nil
 		},
 	})
 
@@ -517,6 +612,7 @@ func TestNamedParametersDriveFlags(t *testing.T) {
 	res, err := (&Client{Path: path}).Invoke("report",
 		map[string]string{"verbose": "true", "label": "here"},
 		[]string{"an-operand"},
+		"",
 	)
 	require.NoError(t, err)
 
@@ -536,12 +632,12 @@ func TestUnknownParameterIsReported(t *testing.T) {
 			fl.Bool("verbose", false, "say more")
 			return fl, nil
 		},
-		Callback: func(fs any, a []string, w commands.StringWriter) error { return nil },
+		Run: func(fs any, a []string) (commands.Result, error) { return commands.Nothing(), nil },
 	})
 
 	_, path := serve(t, reg)
 
-	res, err := (&Client{Path: path}).Invoke("report", map[string]string{"verbos": "true"}, nil)
+	res, err := (&Client{Path: path}).Invoke("report", map[string]string{"verbos": "true"}, nil, "")
 	require.NoError(t, err)
 
 	assert.Contains(t, res.Error, `report has no parameter "verbos"`)
@@ -550,7 +646,7 @@ func TestUnknownParameterIsReported(t *testing.T) {
 func TestParametersOnACommandThatTakesNoneAreReported(t *testing.T) {
 	_, path := serve(t, basicRegistry())
 
-	res, err := (&Client{Path: path}).Invoke("echo", map[string]string{"nope": "true"}, nil)
+	res, err := (&Client{Path: path}).Invoke("echo", map[string]string{"nope": "true"}, nil, "")
 	require.NoError(t, err)
 
 	assert.Contains(t, res.Error, "echo takes no parameters")
@@ -563,14 +659,14 @@ func TestJSONOutputIsAJSONValue(t *testing.T) {
 	reg.Register(&commands.Command{
 		Name:             "emit",
 		ShortDescription: "emits json",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return w.Write(`{"hosts":[1,2,3]}`)
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value(map[string]any{"hosts": []int{1, 2, 3}}), nil
 		},
 	})
 
 	_, path := serve(t, reg)
 
-	res, err := (&Client{Path: path}).Invoke("emit", nil, nil)
+	res, err := (&Client{Path: path}).Invoke("emit", nil, nil, "")
 	require.NoError(t, err)
 
 	var decoded struct {
@@ -584,7 +680,7 @@ func TestJSONOutputIsAJSONValue(t *testing.T) {
 func TestTextOutputIsAJSONString(t *testing.T) {
 	_, path := serve(t, basicRegistry())
 
-	res, err := (&Client{Path: path}).Invoke("echo", nil, []string{"hi"})
+	res, err := (&Client{Path: path}).Invoke("echo", nil, []string{"hi"}, "")
 	require.NoError(t, err)
 
 	var s string
@@ -607,6 +703,88 @@ func TestDescribeReportsFlagTypes(t *testing.T) {
 	assert.NotContains(t, names, "withheld", "the schema advertises only what this surface serves")
 }
 
+// TestPresentationParametersAreRefused: the schema lists json and pretty on every
+// command, because the command lines take them, but a request has one way to choose its
+// rendering, the format field. A client that sends them as parameters is pointed there
+// rather than given a second, weaker way to say the same thing.
+func TestPresentationParametersAreRefused(t *testing.T) {
+	reg := commands.New()
+	reg.Register(&commands.Command{
+		Name:             "pair",
+		ShortDescription: "returns a pair",
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value(map[string]int{"a": 1}), nil
+		},
+		Text: func(v any, w io.Writer) error {
+			_, err := fmt.Fprintln(w, "a is one")
+			return err
+		},
+	})
+	_, path := serve(t, reg)
+	c := &Client{Path: path}
+
+	for _, name := range []string{"json", "pretty"} {
+		res, err := c.Invoke("pair", map[string]string{name: "true"}, nil, "text")
+		require.NoError(t, err)
+		assert.Contains(t, res.Error, name+" is a command line flag, not a parameter")
+		assert.Contains(t, res.Error, "format")
+		assert.Empty(t, res.Result, "a refused request runs nothing")
+	}
+
+	// The format field is the one that counts.
+	res, err := c.Invoke("pair", nil, nil, "text")
+	require.NoError(t, err)
+	require.Empty(t, res.Error)
+	assert.Equal(t, "a is one\n", Text(res.Result))
+
+	res, err = c.Invoke("pair", nil, nil, "")
+	require.NoError(t, err)
+	require.Empty(t, res.Error)
+	assert.JSONEq(t, `{"a":1}`, string(res.Result), "the API default is JSON")
+}
+
+// TestTextRequestToATextlessCommand: a command with no human rendering still answers a
+// text request when the answer is a refusal, since a refusal always has one. Only a
+// successful result has nothing to say in text, and that is reported then, by the daemon.
+func TestTextRequestToATextlessCommand(t *testing.T) {
+	reg := commands.New()
+	reg.Register(&commands.Command{
+		Name:             "tunnel",
+		ShortDescription: "data on success, a refusal without an operand",
+		Run: func(fs any, a []string) (commands.Result, error) {
+			if len(a) == 0 {
+				return commands.Failed("No vpn address was provided"), nil
+			}
+			return commands.Value(map[string]int{"index": 7}), nil
+		},
+		JSONOnly: true,
+	})
+	_, path := serve(t, reg)
+	c := &Client{Path: path}
+
+	res, err := c.Invoke("tunnel", nil, nil, "text")
+	require.NoError(t, err)
+	assert.Equal(t, "No vpn address was provided\n", res.Error, "the refusal is rendered, not the missing text form")
+
+	res, err = c.Invoke("tunnel", nil, []string{"10.0.0.1"}, "text")
+	require.NoError(t, err)
+	assert.Contains(t, res.Error, "tunnel has no human readable output yet; pass -json or -pretty")
+	assert.Empty(t, res.Result)
+
+	res, err = c.Invoke("tunnel", nil, []string{"10.0.0.1"}, "json")
+	require.NoError(t, err)
+	require.Empty(t, res.Error)
+	assert.JSONEq(t, `{"index":7}`, string(res.Result))
+
+	// A refusal asked for as JSON carries the outcome for a program and the reason in
+	// Error, so a client exits non-zero on the envelope alone, never by sniffing the
+	// payload for an ok field some unrelated result might also have.
+	res, err = c.Invoke("tunnel", nil, nil, "json")
+	require.NoError(t, err)
+	assert.Equal(t, "No vpn address was provided", res.Error)
+	assert.JSONEq(t, `{"ok":false,"reason":"No vpn address was provided"}`, string(res.Result))
+}
+
 // TestRebindAfterFailedBindStillStopsOnCancel covers a daemon whose initial listen
 // failed and which later Rebinds onto a good path from a reloaded config. The context
 // watcher has to be installed by that Rebind, or the socket outlives the daemon.
@@ -644,8 +822,8 @@ func TestCommandRegisteredAfterStartIsServed(t *testing.T) {
 	reg.Register(&commands.Command{
 		Name:             "late",
 		ShortDescription: "registered after the server started",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			return w.Write("late")
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Value("late"), nil
 		},
 	})
 
@@ -659,12 +837,50 @@ func TestCommandRegisteredAfterStartIsServed(t *testing.T) {
 	}
 	assert.Contains(t, names, "late")
 
-	res, err := c.Invoke("late", nil, nil)
+	res, err := c.Invoke("late", nil, nil, "")
 	require.NoError(t, err)
 	require.Empty(t, res.Error)
 	assert.JSONEq(t, `"late"`, string(res.Result))
 }
 
+// TestAfterReplyRunsOnceTheResponseIsOut: the hook blocks until the test releases it, so
+// the only way a response can arrive is if it was written before the hook began.
+func TestAfterReplyRunsOnceTheResponseIsOut(t *testing.T) {
+	release := make(chan struct{})
+	acted := make(chan struct{})
+
+	reg := commands.New()
+	reg.Register(&commands.Command{
+		Name:             "act",
+		ShortDescription: "replies, then acts",
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Did("about to act"), nil
+		},
+		AfterReply: func() {
+			<-release
+			close(acted)
+		},
+	})
+	_, path := serve(t, reg)
+
+	res, err := (&Client{Path: path}).Invoke("act", nil, nil, "")
+	require.NoError(t, err)
+	require.Empty(t, res.Error)
+
+	select {
+	case <-acted:
+		t.Fatal("AfterReply ran before the response was delivered")
+	default:
+	}
+
+	close(release)
+	select {
+	case <-acted:
+	case <-time.After(2 * time.Second):
+		t.Fatal("AfterReply never ran")
+	}
+}
+
 // TestRebindRefusesDuringShutdown: a reload that lands after the daemon's context is
 // cancelled must not bring the endpoint back. Stop is not terminal, so Rebind has to
 // take the answer from the context rather than from the running flag.

ssh_commands_test.go

@@ -4,7 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"path/filepath"
-	"strings"
+	"sort"
 	"testing"
 
 	"github.com/slackhq/nebula/commands"
@@ -17,8 +17,9 @@ import (
 // It pins the registered command set, their descriptions, help text and flag definitions
 // so that moving the registry to a transport neutral package is provably a relocation
 // rather than a rewrite. Regenerate with `go test . -run TestSSHCommandSurface -update`.
+
 func TestSSHCommandSurface(t *testing.T) {
-	got := renderCommandSurface(registeredCommands(t).All())
+	got := renderCommandSurface(registeredCommands(t).Schema())
 	test.Golden(t, filepath.Join("testdata", "ssh_commands.golden"), got)
 }
 
@@ -32,29 +33,41 @@ func registeredCommands(t *testing.T) *commands.Registry {
 	return reg
 }
 
-func renderCommandSurface(cmds []*commands.Command) string {
+// renderCommandSurface prints what a client actually sees, which is the schema rather
+// than the Command structs: the presentation flags are offered on every command that
+// returns a value, so reading Command.Flags alone would under-report the surface.
+func renderCommandSurface(cmds []commands.CommandSchema) string {
 	lines := make([]string, 0, len(cmds))
 	for _, cmd := range cmds {
 		b := &bytes.Buffer{}
 		fmt.Fprintf(b, "%s\n", cmd.Name)
-		fmt.Fprintf(b, "  short: %s\n", cmd.ShortDescription)
+		fmt.Fprintf(b, "  short: %s\n", cmd.Short)
 		fmt.Fprintf(b, "  help: %s\n", cmd.Help)
 
-		if cmd.Flags != nil {
-			fl, _ := cmd.Flags()
-			if fl != nil {
-				fb := &bytes.Buffer{}
-				fl.SetOutput(fb)
-				fl.PrintDefaults()
-				fmt.Fprintf(b, "  flags:\n%s", fb.String())
+		if len(cmd.Flags) > 0 {
+			fmt.Fprintf(b, "  flags:\n")
+			for _, f := range cmd.Flags {
+				kind := " string"
+				if f.Bool {
+					kind = ""
+				}
+				fmt.Fprintf(b, "  -%s%s\n    \t%s\n", f.Name, kind, f.Usage)
 			}
 		}
 
 		lines = append(lines, b.String())
 	}
 
-	// In the order All gives, so the golden pins display order too.
-	return strings.Join(lines, "")
+	sort.Strings(lines)
+	return joinLines(lines)
+}
+
+func joinLines(in []string) string {
+	b := &bytes.Buffer{}
+	for _, l := range in {
+		b.WriteString(l)
+	}
+	return b.String()
 }
 
 // TestLocalSocketSurface pins which of nebula's commands the always-on local socket

ssh_reload_test.go

@@ -1,33 +1,17 @@
 package nebula
 
 import (
-	"errors"
-	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
-	"github.com/slackhq/nebula/commands"
 	"github.com/slackhq/nebula/config"
 	"github.com/slackhq/nebula/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-type lineWriter struct {
-	lines []string
-	err   error
-}
-
-func (w *lineWriter) WriteLine(s string) error {
-	w.lines = append(w.lines, s)
-	return w.err
-}
-func (w *lineWriter) Write(s string) error      { return w.WriteLine(s) }
-func (w *lineWriter) WriteBytes(b []byte) error { return w.WriteLine(string(b)) }
-func (w *lineWriter) GetWriter() io.Writer      { return io.Discard }
-func (w *lineWriter) Err() io.Writer            { return io.Discard }
-
 func TestReloadCommandReportsTheResult(t *testing.T) {
 	dir := t.TempDir()
 	write := func(body string) {
@@ -45,51 +29,28 @@ func TestReloadCommandReportsTheResult(t *testing.T) {
 		}
 	})
 
-	// reload returns what the client was told. A reload that failed is a commands.Failure
-	// to the surface, exit 1 over ssh, with the result line as its message; one that
-	// merely needs a restart is not.
-	reload := func(failed bool) []string {
-		w := &lineWriter{}
-		err := reloadCommand(l, c, w)
-		if failed {
-			var f *commands.Failure
-			require.ErrorAs(t, err, &f)
-			assert.Equal(t, w.lines[len(w.lines)-1], f.Msg)
-		} else {
-			require.NoError(t, err)
-		}
-		return w.lines
+	// reload reports what happened. A reload that failed is a refusal, exit 1 over ssh;
+	// one that merely needs a restart is not.
+	reload := func(failed bool) string {
+		res, err := runReload(l, c)
+		require.NoError(t, err)
+		assert.Equal(t, !failed, res.OK)
+		return strings.TrimPrefix(res.Reason, "Reloading config\n")
 	}
 
 	write("a: 2\nb: 1")
-	assert.Equal(t, []string{"Reloading config", "Config reload complete"}, reload(false))
+	assert.Equal(t, "Config reload complete", reload(false))
 
 	fail = true
 	write("a: 3\nb: 1")
-	assert.Equal(t, []string{"Reloading config", "Config reload completed with errors, review the logs"}, reload(true))
+	assert.Equal(t, "Config reload completed with errors, review the logs", reload(true))
 
 	fail = false
 	write("a: 3\nb: 2")
-	assert.Equal(t, []string{"Reloading config", "Config reload was completed but requires a restart to fully apply, keys [b]"}, reload(false))
+	assert.Equal(t, "Config reload was completed but requires a restart to fully apply, keys [b]", reload(false))
 
 	write("a: [")
-	lines := reload(true)
-	require.Len(t, lines, 2)
-	assert.Contains(t, lines[1], "Config reload failed, the new config did not load")
-	assert.Contains(t, lines[1], dir, "names the config that did not load")
-}
-
-// A client that went away still gets the reload it asked for
-func TestReloadCommandReloadsWhenTheClientIsGone(t *testing.T) {
-	dir := t.TempDir()
-	require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte("a: 1"), 0o600))
-	l := test.NewLogger()
-	c := config.NewC(l)
-	require.NoError(t, c.Load(dir))
-	reloaded := false
-	c.RegisterReloadCallback(func(*config.C, *config.Reload) { reloaded = true })
-
-	gone := errors.New("client is gone")
-	require.ErrorIs(t, reloadCommand(l, c, &lineWriter{err: gone}), gone)
-	assert.True(t, reloaded)
+	reason := reload(true)
+	assert.Contains(t, reason, "Config reload failed, the new config did not load")
+	assert.Contains(t, reason, dir, "names the config that did not load")
 }

sshd/session.go

@@ -3,6 +3,7 @@ package sshd
 import (
 	"errors"
 	"log/slog"
+	"sort"
 	"strings"
 
 	"github.com/slackhq/nebula/commands"
@@ -25,6 +26,7 @@ func NewSession(reg *commands.Registry, conn *ssh.ServerConn, chans <-chan ssh.N
 		c:        conn,
 		cancel:   cancel,
 	}
+	s.commands.DefaultFormat = DefaultFormat
 
 	s.commands.Register(LogoutCommand(s.Close))
 
@@ -38,13 +40,34 @@ func LogoutCommand(end func()) *commands.Command {
 	return &commands.Command{
 		Name:             commands.Logout,
 		ShortDescription: "Ends the current session",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
+		Run: func(a any, args []string) (commands.Result, error) {
 			end()
-			return nil
+			return commands.Nothing(), nil
 		},
 	}
 }
 
+// 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
+// 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.
+	if r.Value == nil {
+		return commands.FormatText
+	}
+
+	return commands.FormatJSON
+}
+
 func (s *session) handleChannels(chans <-chan ssh.NewChannel) {
 	defer s.Close()
 	for newChannel := range chans {
@@ -91,7 +114,7 @@ func (s *session) handleRequests(in <-chan *ssh.Request, channel ssh.Channel) {
 			}
 
 			req.Reply(true, nil)
-			err := s.dispatchCommand(payload.Value, commands.NewStringWriter(channel))
+			err := s.dispatchCommand(payload.Value, commands.MergedOutput(channel))
 
 			// A command that failed exits 1, so `ssh host cmd` callers can tell.
 			status := struct{ Status uint32 }{uint32(0)}
@@ -124,6 +147,7 @@ func (s *session) createTerm(channel ssh.Channel) *term.Terminal {
 				return cmds[0] + " ", len(cmds[0]) + 1, true
 			}
 
+			sort.Strings(cmds)
 			term.Write([]byte(strings.Join(cmds, "\n") + "\n\n"))
 		}
 
@@ -135,7 +159,7 @@ func (s *session) createTerm(channel ssh.Channel) *term.Terminal {
 }
 
 func (s *session) handleInput() {
-	w := commands.NewStringWriter(s.term)
+	w := commands.MergedOutput(s.term)
 	for {
 		line, err := s.term.ReadLine()
 		if err != nil {
@@ -149,7 +173,7 @@ func (s *session) handleInput() {
 // dispatchCommand runs line and returns the command's error, which it has logged unless
 // the command already told the user everything: the log line is for the operator reading
 // the daemon's log, who learns nothing from a failure the user caused.
-func (s *session) dispatchCommand(line string, w commands.StringWriter) error {
+func (s *session) dispatchCommand(line string, w commands.Output) error {
 	err := s.commands.Dispatch(line, w)
 	if err == nil {
 		return nil

sshd_reload_test.go

@@ -6,7 +6,6 @@ import (
 	"crypto/rand"
 	"crypto/rsa"
 	"encoding/pem"
-	"errors"
 	"fmt"
 	"net"
 	"os"
@@ -250,9 +249,8 @@ func TestSSHExecExitStatusReflectsTheCommand(t *testing.T) {
 	h := newSSHDReloadHarness(t)
 	h.reg.Register(&commands.Command{
 		Name: "fails",
-		Callback: func(fs any, a []string, w commands.StringWriter) error {
-			_ = w.WriteLine("nope")
-			return errors.New("failed")
+		Run: func(fs any, a []string) (commands.Result, error) {
+			return commands.Failed("nope"), nil
 		},
 	})
 	client, _ := h.connect(t)
@@ -269,7 +267,7 @@ func TestSSHExecExitStatusReflectsTheCommand(t *testing.T) {
 	require.NoError(t, err)
 	out, err = sess.CombinedOutput("help fails")
 	require.NoError(t, err)
-	assert.Equal(t, "fails - \n", string(out))
+	assert.Contains(t, string(out), "fails - \n")
 
 	// A word that is not a command is a failure too, with the command list as before.
 	sess, err = client.NewSession()

testdata/ssh_commands.golden

@@ -4,29 +4,46 @@ change-remote
   flags:
   -address string
     	The new remote address, ip:port
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 close-tunnel
   short: Closes a tunnel for the provided vpn addr
   help: 
   flags:
   -local-only
     	Disables notifying the remote that the tunnel is shutting down
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 create-tunnel
   short: Creates a tunnel for the provided vpn address
   help: The lighthouses will be queried for real addresses but you can provide one as well.
   flags:
   -address string
     	Optionally provide a real remote address, ip:port 
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 device-info
   short: Prints information about the network device.
   help: 
   flags:
   -json
-    	outputs as json with more information
+    	outputs as json
   -pretty
     	pretty prints json, assumes -json
 help
   short: prints available commands or help <command> for specific usage info
   help: 
+  flags:
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 list-hostmap
   short: List all known previously connected hosts
   help: 
@@ -34,7 +51,7 @@ list-hostmap
   -by-index
     	gets all hosts in the hostmap from the index table
   -json
-    	outputs as json with more information
+    	outputs as json
   -pretty
     	pretty prints json, assumes -json
 list-lighthouse-addrmap
@@ -42,7 +59,7 @@ list-lighthouse-addrmap
   help: 
   flags:
   -json
-    	outputs as json with more information
+    	outputs as json
   -pretty
     	pretty prints json, assumes -json
 list-pending-hostmap
@@ -52,43 +69,72 @@ list-pending-hostmap
   -by-index
     	gets all hosts in the hostmap from the index table
   -json
-    	outputs as json with more information
+    	outputs as json
   -pretty
     	pretty prints json, assumes -json
 log-format
   short: Gets or sets the current log format
   help: 
+  flags:
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 log-level
   short: Gets or sets the current log level
   help: 
+  flags:
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 print-cert
   short: Prints the current certificate being used or the certificate for the provided vpn addr
   help: 
   flags:
+  -raw
+    	prints the PEM encoded certificate; with -json, an object holding it
   -json
     	outputs as json
   -pretty
     	pretty prints json, assumes -json
-  -raw
-    	raw prints the PEM encoded certificate, not compatible with -json or -pretty
 print-relays
   short: Prints json details about all relay info
   help: 
   flags:
+  -json
+    	outputs as json
   -pretty
-    	pretty prints json
+    	pretty prints json, assumes -json
 print-tunnel
   short: Prints json details about a tunnel for the provided vpn addr
   help: 
   flags:
+  -json
+    	outputs as json
   -pretty
-    	pretty prints json
+    	pretty prints json, assumes -json
 query-lighthouse
   short: Query the lighthouses for the provided vpn address
   help: This command is asynchronous. Only currently known udp addresses will be printed.
+  flags:
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 reload
   short: Reloads configuration from disk, same as sending HUP to the process
   help: 
+  flags:
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json
 version
   short: Prints the currently running version of nebula
   help: 
+  flags:
+  -json
+    	outputs as json
+  -pretty
+    	pretty prints json, assumes -json