Report ssh command failures to the surface (#1959)

Report ssh command failures to the surface (#1959)

The ssh debug commands told the user when something went wrong but
never told the surface that ran them, so `ssh host cmd` always exited 0
and a few failures were not reported at all. A failed command now
returns its error, a failure the user has already heard about is a
commands.Failure, and the ssh exec path exits 1 for either. The command
bodies also lose their ssh prefixes and the duplication they carried
over from ssh.go; both goldens pin that nothing else changed.

- Dispatch returns the callback's error, or shlex's; the ssh session
  logs it at debug, and not at all for a Failure the user already saw
- commands.Fail writes a message and returns it as a Failure in one step;
  an unknown command, `help` for a command that is not there, a failed
  reload and an unbalanced quote are failures
- ssh exec exit-status is 1 for a command, flag or failure error, 0
  otherwise; the e2e golden records the exit status for every case
- Clone rebinds help to the copy, so an ssh session's help lists logout
  and `help logout` and `logout -h` answer; sshd.LogoutCommand is the
  session's logout, exported for tests
- "did not understand" names the first word only, the one looked up;
  DispatchArgs takes just the tokens
- log-format writes its refusal to the user, as log-level does
- print-relays emits the relay error as a string and omits the key when
  there is none, where it printed {} or "Error":null
- list-hostmap and list-lighthouse-addrmap -json return the encode
  error; print-cert reports a certificate that will not marshal
- a flags type that is not the one registered is an internal error, not
  a silent success
- command functions and flag types are <name>Command and <name>Flags;
  parseVpnAddr and listHostMapFlagSet replace the repeated parse block
  and flag set; close-tunnel sends through sendCloseTunnel

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

commands/command.go

@@ -9,12 +9,14 @@ import (
 type CommandFlags func() (*flag.FlagSet, any)
 
 // CommandCallback is called when your command should execute.
-// fs will be a a pointer to the struct provided by Command.Flags callback, if there was one. -h and -help are reserved
+// 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.
-// If an error is returned by the callback it is logged locally, the callback should handle messaging errors to the user
-// where appropriate
+// 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
 
 type Command struct {
@@ -26,6 +28,27 @@ type Command struct {
 	Callback CommandCallback
 }
 
+// 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.
+type Failure struct {
+	Msg string
+}
+
+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 {
+		return err
+	}
+	return &Failure{Msg: msg}
+}
+
 func execCommand(c *Command, args []string, w StringWriter) error {
 	var (
 		fl *flag.FlagSet

commands/registry.go

@@ -38,7 +38,8 @@ func (r *Registry) registerHelp() {
 	})
 }
 
-// Register adds a command that can be run by a user.
+// 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.
 func (r *Registry) Register(c *Command) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
@@ -77,29 +78,32 @@ func (r *Registry) All() []*Command {
 	return cmds
 }
 
-// Clone returns a copy holding the same commands.
+// 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.
 func (r *Registry) Clone() *Registry {
 	r.mu.RLock()
-	defer r.mu.RUnlock()
-	return &Registry{commands: maps.Clone(r.commands)}
+	c := &Registry{commands: maps.Clone(r.commands)}
+	r.mu.RUnlock()
+	c.registerHelp()
+	return c
 }
 
-// Dispatch parses a command line and runs it, reporting problems to w.
-func (r *Registry) Dispatch(line string, w StringWriter) {
-	// A line shlex cannot split, an unbalanced quote, gets no answer. sshd was silent
-	// here too; this is a relocation, so it stays silent.
+// 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 {
+	// A line shlex cannot split, an unbalanced quote.
 	args, err := shlex.Split(line, true)
 	if err != nil {
-		return
+		return Fail(w, "could not parse the command line: "+err.Error())
 	}
 
-	_ = r.DispatchArgs(args, line, w)
+	return r.DispatchArgs(args, w)
 }
 
-// DispatchArgs runs an already tokenized command line. orig is what the user typed,
-// used only for the "did not understand" message; callers that have no raw line may
-// pass "" and the joined args are used instead.
-func (r *Registry) DispatchArgs(args []string, orig 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 {
 	if len(args) == 0 {
 		r.dumpCommands(w)
 		return nil
@@ -107,16 +111,14 @@ func (r *Registry) DispatchArgs(args []string, orig string, w StringWriter) erro
 
 	c := r.Get(Name(args[0]))
 	if c == nil {
-		if orig == "" {
-			orig = strings.Join(args, " ")
-		}
-
-		if err := w.WriteLine(fmt.Sprintf("did not understand: %s", orig)); err != nil {
+		// 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 {
 			return err
 		}
 
 		r.dumpCommands(w)
-		return nil
+		return &Failure{Msg: msg}
 	}
 
 	if checkHelpArgs(args) {
@@ -154,7 +156,7 @@ func (r *Registry) help(a []string, w StringWriter) error {
 	// We are printing a specific commands help text
 	cmd := r.Get(Name(a[0]))
 	if cmd == nil {
-		return w.WriteLine("Command not available " + a[0])
+		return Fail(w, "Command not available "+a[0])
 	}
 
 	if err := w.WriteLine(fmt.Sprintf("%s - %s", cmd.Name, cmd.ShortDescription)); err != nil {

commands/registry_test.go

@@ -2,6 +2,7 @@ package commands
 
 import (
 	"bytes"
+	"errors"
 	"flag"
 	"strings"
 	"testing"
@@ -45,18 +46,41 @@ func TestFlagUsageGoesToTheWriter(t *testing.T) {
 	})
 
 	merged := &bytes.Buffer{}
-	require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, "", NewStringWriter(merged)))
+	require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, NewStringWriter(merged)))
 	assert.Contains(t, merged.String(), "flag provided but not defined")
 }
 
-// TestDispatchDropsUnparseableLine pins sshd's behaviour for a line shlex rejects: no
-// answer at all. An operator with an unbalanced quote gets silence, as they always have.
-func TestDispatchDropsUnparseableLine(t *testing.T) {
+// TestDispatchReportsUnparseableLine: a line shlex rejects, an unbalanced quote, is a
+// failure the user is told about.
+func TestDispatchReportsUnparseableLine(t *testing.T) {
 	out := &bytes.Buffer{}
-	testRegistry().Dispatch(`served "unterminated`, NewStringWriter(out))
-	assert.Empty(t, out.String())
+	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: ")
 }
 
+// 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)
+}
+
+type failingWriter struct{}
+
+func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("gone") }
+
 // TestAllSortsByName: registration order does not matter.
 func TestAllSortsByName(t *testing.T) {
 	r := New()
@@ -72,8 +96,8 @@ func TestAllSortsByName(t *testing.T) {
 }
 
 // TestHelpFlagAgreesWithHelpCommand pins that -h answers through the registered help
-// command. On a clone, help is bound to the source, so a command registered only on the
-// clone is known to neither -h nor `help <name>`, and the two never disagree.
+// 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.
 func TestHelpFlagAgreesWithHelpCommand(t *testing.T) {
 	r := testRegistry()
 	c := r.Clone()
@@ -88,10 +112,65 @@ func TestHelpFlagAgreesWithHelpCommand(t *testing.T) {
 	cmd := &bytes.Buffer{}
 	c.Dispatch("help logout", NewStringWriter(cmd))
 	assert.Equal(t, cmd.String(), flag.String())
-	assert.Equal(t, "Command not available logout\n", 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())
 }
+
+// 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()
+
+	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())
+
+	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())
+}
+
+// TestDispatchReturnsTheCommandsError: what the callback returns reaches the caller, so
+// a surface can log it.
+func TestDispatchReturnsTheCommandsError(t *testing.T) {
+	r := New()
+	r.Register(&Command{
+		Name: "fails",
+		Callback: func(fs any, a []string, w StringWriter) error {
+			return errors.New("boom")
+		},
+	})
+
+	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")
+}
+
+// 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) {
+	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())
+}