fix(redis): validate CLIENT subcommand arity + distinguish string/bulk in test stub by bootjp · Pull Request #608 · bootjp/elastickv

bootjp · 2026-04-23T19:23:41Z

Addresses coderabbit review on f3a1903.

MAJOR: CLIENT subcommand arity (adapter/redis_compat_commands.go)

argsLen only guarantees CLIENT has one subcommand, so malformed requests silently succeeded:

CLIENT SETNAME (missing name) -> OK, no mutation
CLIENT GETNAME extra (extra operand) -> name as if no operand
CLIENT ID junk -> ID returned
CLIENT INFO extra -> INFO returned

Each subcommand now validates its exact arg count via a shared checkClientArity helper, returning
ERR wrong number of arguments for 'client|<sub>' command on mismatch. Each case was also extracted into its own method so the switch body stays under the cyclomatic budget.

MINOR: distinguish string from bulk in helloRecordingConn test stub (adapter/redis_hello_test.go)

WriteString was recorded as kind="bulk", which meant a future regression swapping WriteBulkString for WriteString in the HELLO reply path would not fail any test. Now recorded as kind="string"; no existing assertions broke since all HELLO key/value writes use WriteBulk / WriteBulkString.

Regression tests added: CLIENT SETNAME / GETNAME / ID / INFO arity checks (+4 tests). go test ./adapter -race -run TestClient_|TestHello_ passes; golangci-lint clean.

…k in test stub Addresses coderabbit review on f3a1903. MAJOR: CLIENT subcommand arity (adapter/redis_compat_commands.go) `argsLen` only guarantees CLIENT has one subcommand, so malformed requests silently succeeded: CLIENT SETNAME (missing name) -> OK, no mutation CLIENT GETNAME extra (extra operand) -> name as if no operand CLIENT ID junk -> ID returned CLIENT INFO extra -> INFO returned Each subcommand now validates its exact arg count via a shared checkClientArity helper, returning `ERR wrong number of arguments for 'client|<sub>' command` on mismatch. Each case was also extracted into its own method so the switch body stays under the cyclomatic budget. MINOR: distinguish string from bulk in helloRecordingConn test stub (adapter/redis_hello_test.go) WriteString was recorded as kind="bulk", which meant a future regression swapping WriteBulkString for WriteString in the HELLO reply path would not fail any test. Now recorded as kind="string"; no existing assertions broke since all HELLO key/value writes use WriteBulk / WriteBulkString. Regression tests added: CLIENT SETNAME / GETNAME / ID / INFO arity checks (+4 tests). go test ./adapter -race -run TestClient_|TestHello_ passes; golangci-lint clean.

gemini-code-assist · 2026-04-23T19:23:45Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

coderabbitai · 2026-04-23T19:23:48Z

Warning

Rate limit exceeded

@bootjp has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 32 minutes and 17 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 17 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 03fb05dc-11b9-400d-a61b-09813d69d619

📥 Commits

Reviewing files that changed from the base of the PR and between 5b4af15 and 7d283f6.

📒 Files selected for processing (2)

adapter/redis_compat_commands.go
adapter/redis_hello_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/redis-hello-command

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Tightens Redis adapter compatibility by enforcing correct CLIENT subcommand argument counts and improving test stubs to better detect RESP wire-shape regressions.

Changes:

Add exact-arity validation for CLIENT SETNAME/GETNAME/ID/INFO via a shared helper and extract handlers to reduce complexity in the main switch.
Update helloRecordingConn to record WriteString as a distinct kind from bulk strings.
Add regression tests covering wrong-arity CLIENT subcommand calls.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`adapter/redis_compat_commands.go`	Introduces `checkClientArity` and refactors `CLIENT` subcommand handling to reject malformed arities.
`adapter/redis_hello_test.go`	Improves HELLO reply recording fidelity and adds arity regression tests for `CLIENT` subcommands.

Copilot · 2026-04-24T17:16:24Z

 	case "SETINFO":
 		conn.WriteString("OK")
 	case "SETNAME":


CLIENT SETINFO still returns OK for any arity (including missing operands). This leaves one CLIENT subcommand with the same silent-success behavior this PR is trying to eliminate and diverges from Redis’ wrong-arity behavior for client|setinfo. Consider validating SETINFO’s expected argument count (or at least rejecting obviously wrong arity) using checkClientArity for consistency.

Copilot · 2026-04-24T17:16:25Z

+	if checkClientArity(conn, cmd, "SETNAME", clientSetNameMinArgs) {
+		return
+	}


checkClientArity enforces an exact argument count, but the constant name clientSetNameMinArgs suggests a minimum and can be misread/reused incorrectly later. Rename it to reflect exact arity (e.g., clientSetNameArgCount) or introduce a dedicated exact-arity constant to match the semantics here.

Copilot · 2026-04-24T17:16:25Z

+	r.client(conn, redcon.Command{Args: [][]byte{
+		[]byte("CLIENT"), []byte("SETNAME"),
+	}})


These tests hardcode []byte("CLIENT") even though this file already uses the shared cmdClient constant elsewhere. Using []byte(cmdClient) keeps command spelling consistent and avoids drift/typos if the command constants change.

Copilot review on PR #608: - CLIENT SETINFO still returned OK for any arity including missing operands. That was the last CLIENT subcommand keeping the silent- success behaviour this PR was supposed to eliminate, and it diverged from the real Redis wrong-arity reply for client|setinfo. Now routed through checkClientArity with clientSetInfoArgCount=4 (CLIENT + SETINFO + attr + value), mirroring SETNAME. - Rename clientSetNameMinArgs to clientSetNameArgCount. The helper compares len(cmd.Args) == want, so the "Min" suffix was misleading and invited a future refactor that swaps the helper for a >= check and silently re-introduces the wrong-arity bug. - Replace hardcoded []byte("CLIENT") in the new arity tests with []byte(cmdClient) to stay consistent with the rest of the file and keep the command name in one place. New TestClient_SetInfo_RejectsWrongArity pins all four arity shapes (2, 3, 4, 5 args) so the SETINFO regression cannot be reintroduced.

bootjp · 2026-04-24T21:16:18Z

Addressed Copilot review (all 3 comments) in 7d283f6:

CLIENT SETINFO arity enforcement. SETINFO was still returning OK for any arity (including missing operands), the last remaining silent-success CLIENT subcommand — exactly what this PR is supposed to eliminate. Now routed through checkClientArity with clientSetInfoArgCount = 4 (CLIENT + SETINFO + attr + value), mirroring SETNAME. New TestClient_SetInfo_RejectsWrongArity pins all four arity shapes (2 / 3 / 4 / 5 args).

Constant name clarity. Renamed clientSetNameMinArgs to clientSetNameArgCount. The helper compares len(cmd.Args) == want, so the "Min" suffix misrepresented the semantics and invited a future refactor that could swap the helper for a >= check and silently re-introduce the wrong-arity bug. Added clientSetInfoArgCount as a separate constant for the same reason.

Test consistency. Replaced hardcoded []byte("CLIENT") in the arity tests with []byte(cmdClient) so the command name stays in one place.

/gemini review
@codex review

gemini-code-assist · 2026-04-24T21:16:21Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

chatgpt-codex-connector · 2026-04-24T21:19:02Z

Codex Review: Didn't find any major issues. 👍

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

bootjp requested a review from Copilot April 24, 2026 17:12

Copilot started reviewing on behalf of bootjp April 24, 2026 17:12 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

fix(redis): validate CLIENT subcommand arity + distinguish string/bulk in test stub#608
bootjp wants to merge 2 commits intomainfrom
feat/redis-hello-command

bootjp commented Apr 23, 2026

Uh oh!

gemini-code-assist Bot commented Apr 23, 2026

Uh oh!

coderabbitai Bot commented Apr 23, 2026 •

edited

Loading

Rate limit exceeded

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

bootjp commented Apr 24, 2026

Uh oh!

gemini-code-assist Bot commented Apr 24, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bootjp commented Apr 23, 2026

Uh oh!

gemini-code-assist Bot commented Apr 23, 2026

Uh oh!

coderabbitai Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

bootjp commented Apr 24, 2026

Uh oh!

gemini-code-assist Bot commented Apr 24, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Apr 23, 2026 •

edited

Loading