fix(redis): validate CLIENT subcommand arity + distinguish string/bulk in test stub

adapter/redis_compat_commands.go

@@ -206,32 +206,87 @@ func (r *RedisServer) setnx(conn redcon.Conn, cmd redcon.Command) {
 	conn.WriteInt(1)
 }
 
+// clientSubcommandArgCount is the total cmd.Args length (including
+// CLIENT + subcommand) required by no-operand CLIENT subcommands
+// like GETNAME / ID / INFO.
+const clientSubcommandArgCount = 2
+
+// checkClientArity verifies cmd.Args has exactly want elements and
+// writes the standard Redis wrong-arity error otherwise. Returns
+// true when the caller should stop handling (bad arity).
+func checkClientArity(conn redcon.Conn, cmd redcon.Command, sub string, want int) bool {
+	if len(cmd.Args) == want {
+		return false
+	}
+	conn.WriteError("ERR wrong number of arguments for 'client|" + strings.ToLower(sub) + "' command")
+	return true
+}
+
+// clientSetName handles CLIENT SETNAME. SETNAME is shared with
+// HELLO's SETNAME clause; both write into the same connState.clientName
+// slot so a client that uses HELLO SETNAME once and then queries
+// CLIENT GETNAME gets the right answer without having to re-issue
+// CLIENT SETNAME.
+func clientSetName(conn redcon.Conn, cmd redcon.Command, state *connState) {
+	if checkClientArity(conn, cmd, "SETNAME", clientSetNameArgCount) {
+		return
+	}
+	state.clientName = string(cmd.Args[2])
+	conn.WriteString("OK")
+}
+
+func clientGetName(conn redcon.Conn, cmd redcon.Command, state *connState) {
+	if checkClientArity(conn, cmd, "GETNAME", clientSubcommandArgCount) {
+		return
+	}
+	if state.clientName == "" {
+		conn.WriteNull()
+		return
+	}
+	conn.WriteBulkString(state.clientName)
+}
+
+func (r *RedisServer) clientID(conn redcon.Conn, cmd redcon.Command, state *connState) {
+	if checkClientArity(conn, cmd, "ID", clientSubcommandArgCount) {
+		return
+	}
+	conn.WriteInt64(int64(r.ensureConnID(state))) //nolint:gosec // connID monotonic counter, guaranteed <= math.MaxInt64 in practice
+}
+
+func (r *RedisServer) clientInfo(conn redcon.Conn, cmd redcon.Command, state *connState) {
+	if checkClientArity(conn, cmd, "INFO", clientSubcommandArgCount) {
+		return
+	}
+	id := r.ensureConnID(state)
+	conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName))
+}
+
+// clientSetInfo handles CLIENT SETINFO <attr> <value>. elastickv does
+// not persist the advertised attributes (lib-name / lib-ver, etc.), but
+// it MUST still enforce exact arity — otherwise `CLIENT SETINFO` with
+// no operands returns OK and masks a client bug that real Redis would
+// have surfaced as a wrong-arity error.
+func clientSetInfo(conn redcon.Conn, cmd redcon.Command) {
+	if checkClientArity(conn, cmd, "SETINFO", clientSetInfoArgCount) {
+		return
+	}
+	conn.WriteString("OK")
+}
+
 func (r *RedisServer) client(conn redcon.Conn, cmd redcon.Command) {
 	sub := strings.ToUpper(string(cmd.Args[1]))
 	state := getConnState(conn)
 	switch sub {
 	case "SETINFO":
-		conn.WriteString("OK")
+		clientSetInfo(conn, cmd)
 	case "SETNAME":
-		// SETNAME is shared with HELLO's SETNAME clause; both write into
-		// the same connState.clientName slot so a client that uses
-		// HELLO SETNAME once and then queries CLIENT GETNAME gets the
-		// right answer without having to re-issue CLIENT SETNAME.
-		if len(cmd.Args) >= clientSetNameMinArgs {
-			state.clientName = string(cmd.Args[2])
-		}
-		conn.WriteString("OK")
+		clientSetName(conn, cmd, state)
 	case "GETNAME":
-		if state.clientName == "" {
-			conn.WriteNull()
-			return
-		}
-		conn.WriteBulkString(state.clientName)
+		clientGetName(conn, cmd, state)
 	case "ID":
-		conn.WriteInt64(int64(r.ensureConnID(state))) //nolint:gosec // connID monotonic counter, guaranteed <= math.MaxInt64 in practice
+		r.clientID(conn, cmd, state)
 	case "INFO":
-		id := r.ensureConnID(state)
-		conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName))
+		r.clientInfo(conn, cmd, state)
 	default:
 		conn.WriteError("ERR unsupported CLIENT subcommand '" + sub + "'")
 	}
@@ -255,9 +310,20 @@ const (
 	// helloReplyArrayLen is the number of elements in the flat
 	// alternating key/value reply: 7 pairs = 14 elements.
 	helloReplyArrayLen = 14
-	// clientSetNameMinArgs is the arg count at which CLIENT SETNAME
-	// has its <name> operand (CLIENT + SETNAME + name = 3).
-	clientSetNameMinArgs = 3
+	// clientSetNameArgCount is the EXACT cmd.Args length for CLIENT
+	// SETNAME (CLIENT + SETNAME + name = 3). Kept as an exact-arity
+	// constant — not a minimum — because checkClientArity compares
+	// `len(cmd.Args) == want`; renaming it to *MinArgs would invite a
+	// future refactor that "just" swaps the helper for a >= check and
+	// silently re-introduces the wrong-arity silent-OK bug.
+	clientSetNameArgCount = 3
+	// clientSetInfoArgCount is the EXACT cmd.Args length for CLIENT
+	// SETINFO (CLIENT + SETINFO + attr + value = 4). Real Redis
+	// rejects any other arity for `client|setinfo`; without this
+	// check we would keep returning OK for `CLIENT SETINFO` with no
+	// operands, matching exactly the silent-success behaviour this
+	// PR is supposed to eliminate for every CLIENT subcommand.
+	clientSetInfoArgCount = 4
 )
 
 // helloParseError is the internal signal used by parseHelloArgs to

adapter/redis_hello_test.go

@@ -22,7 +22,7 @@ import (
 // array-header) is the only way to distinguish `proto` (integer 2)
 // from `version` (bulk "7.0.0") or `modules` (empty array header).
 type helloReplyElement struct {
-	kind string // "bulk" | "int" | "arrayHeader"
+	kind string // "bulk" | "string" | "int" | "arrayHeader" | "null"
 	str  string
 	num  int64
 }
@@ -46,7 +46,10 @@ func (c *helloRecordingConn) Close() error          { return nil }
 func (c *helloRecordingConn) WriteError(msg string) { c.err = msg }
 func (c *helloRecordingConn) WriteString(s string) {
 	c.str = s
-	c.writes = append(c.writes, helloReplyElement{kind: "bulk", str: s})
+	// Recorded as a distinct "string" kind so a future regression that
+	// swaps WriteBulkString for WriteString (or vice versa) fails the
+	// wire-shape assertions instead of passing silently.
+	c.writes = append(c.writes, helloReplyElement{kind: "string", str: s})
 }
 func (c *helloRecordingConn) WriteBulk(bulk []byte) {
 	c.writes = append(c.writes, helloReplyElement{kind: "bulk", str: string(bulk)})
@@ -419,3 +422,110 @@ elastickv_redis_requests_total{command="HELLO",node_address="10.0.0.1:50051",nod
 	)
 	require.NoError(t, err)
 }
+
+func TestClient_SetName_RejectsWrongArity(t *testing.T) {
+	t.Parallel()
+
+	r := newHelloTestServer(t, true)
+	conn := &helloRecordingConn{}
+	state := getConnState(conn)
+	state.clientName = "prev"
+
+	// CLIENT SETNAME with no value (2 args total).
+	r.client(conn, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("SETNAME"),
+	}})
+	require.Contains(t, conn.err, "wrong number of arguments")
+	require.Equal(t, "prev", state.clientName, "malformed SETNAME must not clobber clientName")
+
+	// CLIENT SETNAME foo bar (4 args total) — also rejected.
+	conn2 := &helloRecordingConn{}
+	state2 := getConnState(conn2)
+	state2.clientName = "prev"
+	r.client(conn2, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("SETNAME"), []byte("foo"), []byte("bar"),
+	}})
+	require.Contains(t, conn2.err, "wrong number of arguments")
+	require.Equal(t, "prev", state2.clientName)
+}
+
+func TestClient_GetName_RejectsWrongArity(t *testing.T) {
+	t.Parallel()
+
+	r := newHelloTestServer(t, true)
+	conn := &helloRecordingConn{}
+
+	// CLIENT GETNAME extra (3 args total) — GETNAME takes no operand.
+	r.client(conn, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("GETNAME"), []byte("extra"),
+	}})
+	require.Contains(t, conn.err, "wrong number of arguments")
+}
+
+func TestClient_ID_RejectsWrongArity(t *testing.T) {
+	t.Parallel()
+
+	r := newHelloTestServer(t, true)
+	conn := &helloRecordingConn{}
+
+	// CLIENT ID junk (3 args total) — ID takes no operand.
+	r.client(conn, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("ID"), []byte("junk"),
+	}})
+	require.Contains(t, conn.err, "wrong number of arguments")
+}
+
+func TestClient_Info_RejectsWrongArity(t *testing.T) {
+	t.Parallel()
+
+	r := newHelloTestServer(t, true)
+	conn := &helloRecordingConn{}
+
+	// CLIENT INFO extra (3 args total) — INFO takes no operand.
+	r.client(conn, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("INFO"), []byte("extra"),
+	}})
+	require.Contains(t, conn.err, "wrong number of arguments")
+}
+
+// CLIENT SETINFO must enforce exact arity too; the prior implementation
+// returned OK for any arity including zero operands, which silently
+// hid client bugs that real Redis would reject as wrong-arity. Pin the
+// "missing operands" and "extra operand" cases so a regression cannot
+// re-introduce the silent-success behaviour.
+func TestClient_SetInfo_RejectsWrongArity(t *testing.T) {
+	t.Parallel()
+
+	r := newHelloTestServer(t, true)
+
+	// CLIENT SETINFO with no operands (2 args total).
+	conn := &helloRecordingConn{}
+	r.client(conn, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("SETINFO"),
+	}})
+	require.Contains(t, conn.err, "wrong number of arguments")
+	require.NotEqual(t, "OK", conn.str,
+		"missing-operand SETINFO must not silently return OK")
+
+	// CLIENT SETINFO attr (3 args total) — missing value.
+	conn2 := &helloRecordingConn{}
+	r.client(conn2, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("SETINFO"), []byte("lib-name"),
+	}})
+	require.Contains(t, conn2.err, "wrong number of arguments")
+
+	// CLIENT SETINFO attr value extra (5 args total) — extra operand.
+	conn3 := &helloRecordingConn{}
+	r.client(conn3, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), []byte("redis-go"), []byte("extra"),
+	}})
+	require.Contains(t, conn3.err, "wrong number of arguments")
+
+	// CLIENT SETINFO attr value (4 args total) — accepted shape.
+	conn4 := &helloRecordingConn{}
+	r.client(conn4, redcon.Command{Args: [][]byte{
+		[]byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), []byte("redis-go"),
+	}})
+	require.Empty(t, conn4.err, "well-formed SETINFO must not produce an error")
+	require.Equal(t, "OK", conn4.str, "well-formed SETINFO replies OK")
+}