feat(redis): implement COMMAND (INFO / COUNT / LIST / DOCS / GETKEYS)#607
feat(redis): implement COMMAND (INFO / COUNT / LIST / DOCS / GETKEYS)#607
Conversation
Add the Redis `COMMAND` handler (COUNT / LIST / INFO / DOCS / GETKEYS subcommands) so modern clients (go-redis, redis-py, ioredis) stop polluting the unsupported-command metric at connect time. Metadata lives in a single static table in adapter/redis_command_info.go so adding a new command is a one-line row addition. - Route `COMMAND` in RedisServer.route and register `-1` in argsLen. - Extend monitoring.redisCommandSet so the metric shows the real name. - Return 6-element INFO arrays (name, arity, flags, first/last/step) — enough for every client that consumes COMMAND output. - GETKEYS walks the per-command first_key/last_key/step positionally. - GETKEYSANDFLAGS and LIST FILTERBY variants are explicitly rejected. - Routed-but-missing-from-table commands emit a one-shot warning and a zero-metadata fallback row so the omission is discoverable in prod; TestCommand_RouteMatchesTable is the hard CI gate.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
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 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements support for the Redis Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 046169bd9b
ℹ️ 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".
| if last < 0 { | ||
| // "to end": last arg index is len(argv)-1. | ||
| last = len(argv) - 1 |
There was a problem hiding this comment.
redisCommandGetKeys currently collapses every negative LastKey to len(argv)-1, but the table includes commands like BZPOPMIN with LastKey: -2 to exclude the trailing timeout argument. With the current logic, COMMAND GETKEYS BZPOPMIN k1 k2 0 incorrectly returns 0 as a key, which can break client key-routing decisions. Negative LastKey values need to be handled as relative offsets (e.g., len(argv)+LastKey) instead of always meaning “to end.”
Useful? React with 👍 / 👎.
Codex P1: redisCommandGetKeys collapsed every negative LastKey to "to end" (len(argv)-1), but the command table includes entries like BZPOPMIN with LastKey=-2 to EXCLUDE the trailing blocking timeout arg. With the previous logic, COMMAND GETKEYS BZPOPMIN k1 k2 0 returned 0 as a key, which can misdirect client key-routing decisions. Negative LastKey is now treated as an offset from the end: -1 is the final arg, -2 is the second-to-last, and so on (len(argv)+last). This matches the Redis key-specs semantics. Tests pin the BZPOPMIN case end-to-end via the handler and a helper- level test for synthetic -2 metadata, so a future change that reverts to the "every negative means to end" shortcut trips CI.
|
Addressed Codex P1 in 9a90b7e: Interpret negative LastKey correctly in COMMAND GETKEYS. Negative LastKey is now treated as an offset from the end (len(argv)+last), matching the Redis key-specs semantics. -1 still means the final arg; -2 means second-to-last, and so on. Two new regression tests pin the fix:
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a90b7edd4
ℹ️ 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".
| conn.WriteArray(len(requested)) | ||
| for _, raw := range requested { |
There was a problem hiding this comment.
COMMAND DOCS is defined to return documentation for all commands when no command names are provided, but this implementation sizes the reply from len(requested) and iterates only requested, so COMMAND DOCS (without names) always returns an empty array. That makes the subcommand non-compliant and breaks clients/tools that rely on the default full-docs behavior.
Useful? React with 👍 / 👎.
| conn.WriteArray(docEntryLen) | ||
| conn.WriteBulkString("summary") | ||
| conn.WriteBulkString("") | ||
| conn.WriteBulkString("arguments") | ||
| conn.WriteArray(0) |
There was a problem hiding this comment.
The RESP2 form of COMMAND DOCS is a flattened map where each key is the command name and each value is that command's doc map, but this code writes only the value map fields (summary/arguments) and never writes the command-name key. Clients decoding the response as a map (e.g., command name -> docs) will fail or see malformed data.
Useful? React with 👍 / 👎.
Codex P2 (x2) on COMMAND DOCS wire shape: 1. Bare COMMAND DOCS (no command names) previously returned an empty array — the implementation sized the reply from len(requested) and iterated only the requested slice. Real Redis returns docs for every command in that case (same pattern as bare COMMAND and bare COMMAND INFO), which is what redis-cli --docs and capability- probing clients rely on. Bare DOCS now iterates the routed set via routedRedisCommandMetas(). 2. The RESP2 form of COMMAND DOCS is a flattened map: alternating command-name keys and 4-element doc-map values. Previously we wrote only the value slots (summary / arguments) and never the command- name key, so any client decoding the response as a name -> docs map saw malformed data. Now each entry is a (name, docs) pair; unknown commands emit (name, nil) preserving the flat-map layout. Updated the existing TestCommandDocs_Get to assert the new shape (outer array length 2, name bulk, then doc-map). Added TestCommandDocs_BareReturnsAllDocs pinning the full-set default and TestCommandDocs_UnknownReturnsNamedNil pinning the (name, nil) shape for unknown commands.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed Codex P2 (x2) in fb540a2: Bare COMMAND DOCS now returns docs for all routed commands. The prior implementation sized the reply from len(requested) and iterated only the requested slice, so bare DOCS returned an empty array — non-compliant with real Redis and with how redis-cli --docs and capability-probing clients consume the reply. Bare DOCS now iterates routedRedisCommandMetas(), matching the bare-COMMAND / bare-COMMAND-INFO pattern. Pinned by TestCommandDocs_BareReturnsAllDocs. Command-name keys are now emitted in the RESP2 flat-map shape. Redis replies to COMMAND DOCS with alternating (name, doc-map, name, doc-map, ...) pairs; previously we wrote only the doc-map slots, so any client decoding the response as a name -> docs map saw malformed data. Every entry now writes both the name bulk and the doc-map (or (name, nil) for unknown commands to preserve the flat-map layout). Existing TestCommandDocs_Get updated to pin the new shape; new TestCommandDocs_UnknownReturnsNamedNil covers the unknown-command pair. /gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb540a29b4
ℹ️ 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".
| if meta.FirstKey <= 0 { | ||
| return nil |
There was a problem hiding this comment.
redisCommandGetKeys returns nil as soon as FirstKey <= 0, which makes every movable-key command in the new table (EVAL, EVALSHA, XREAD) look like it has no keys. For example, COMMAND GETKEYS EVAL "..." 2 k1 k2 arg should return k1/k2, but this path falls through to ERR The command has no key arguments in writeCommandGetKeys. That breaks client key-routing flows that rely on server-side key extraction for scripted/dynamic commands.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Redis COMMAND family of subcommands, including COUNT, LIST, INFO, DOCS, and GETKEYS, to support client capability probing. It introduces a centralized metadata table in adapter/redis_command_info.go that defines arity, flags, and key positions for all supported commands. The changes also include comprehensive unit tests and integration into the existing command routing and monitoring systems. I have no feedback to provide.
…mmand # Conflicts: # adapter/redis_compat_commands.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
adapter/redis_command_info.go (1)
196-206: Preferslogoverlog.Printffor the missing-metadata warning.The repo's logging guideline calls for
slogwith structured keys (e.g.command=…), which makes this warning easier to filter/route in production than the current free-formlog.Printfline. Since this is a one-shot warn, the cost of switching is trivial.♻️ Proposed slog refactor
import ( - "log" + "log/slog" "sort" "strings" "sync" ) @@ if !warned { - log.Printf("redis-command: routed command %q has no entry in redisCommandTable; emitting zero-metadata fallback. Add a row to adapter/redis_command_info.go.", upper) + slog.Warn("routed command has no entry in redisCommandTable; emitting zero-metadata fallback", + "command", upper, + "hint", "add a row to adapter/redis_command_info.go") }As per coding guidelines: "Use
slogfor logging and maintain structured logging keys (e.g.,key,commit_ts)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/redis_command_info.go` around lines 196 - 206, Replace the free-form log.Printf call in warnMissingRedisCommandMeta with structured slog usage: call slog.Warn (or slog.Logger.Warn) and include the command value as a structured key (e.g., command=upper) and a short message about missing redis command metadata; keep the one-shot guard logic around redisCommandFallbackWarnedOnce and redisCommandFallbackWarnedOnceMu unchanged. Ensure the file imports slog if not already present.adapter/redis_command_test.go (2)
71-72: Optional: makeWriteAny/WriteRawflag unexpected use rather than silently dropping writes.These are no-ops, which works for the current handler (it uses typed writers exclusively). However, a future refactor that switches any reply path to
WriteAnywould silently bypass every assertion in this file without any failing test. Since the tests already encode the precise wire shape, recording these as{op: "any"}/{op: "raw"}entries (or callingt.Errorffrom inside them via a stored*testing.T) costs little and keeps the trace honest.♻️ Minimal trace-instead-of-drop variant
-func (c *commandRecorder) WriteRaw([]byte) {} -func (c *commandRecorder) WriteAny(any) {} +func (c *commandRecorder) WriteRaw(b []byte) { + c.writes = append(c.writes, commandRecorderEntry{op: "raw", s: string(b)}) +} +func (c *commandRecorder) WriteAny(v any) { + c.writes = append(c.writes, commandRecorderEntry{op: "any", s: fmt.Sprintf("%v", v)}) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/redis_command_test.go` around lines 71 - 72, The no-op implementations of commandRecorder.WriteRaw and commandRecorder.WriteAny should not silently drop data; update commandRecorder to either store a testing.T pointer or otherwise record these invocations so tests fail or reflect the call: change WriteRaw to append an entry like {op:"raw", data:...} to the recorder's trace (or call t.Errorf) and change WriteAny to append {op:"any", value:...} (or similarly report an error) so any unexpected use is detectable; locate these methods on the commandRecorder type and modify them to record the operation and payload instead of doing nothing.
414-425: TightenTestCommand_RouteMatchesTableto assert the route map directly.The comment acknowledges the workaround: iterating
argsLeninstead of the actual route map. SinceNewRedisServer’s setup is the source of truth for routing, you can extract the route registration into a small helper (buildRedisRoutes(*RedisServer) map[string]func(...)) shared betweenNewRedisServerand this test. That would catch a mismatch where a handler is added toroutebut forgotten inargsLen(the current test only catches the converse). Optional under chill review sinceTestCommandCount_MatchesTableSizealready pinslen(argsLen) == len(redisCommandTable).The trailing
_ = srv(Line 424) is dead —srvis never used; please drop it along with the no-op declaration on Line 416.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/redis_command_test.go` around lines 414 - 425, TestCommand_RouteMatchesTable currently inspects argsLen instead of the actual route map and leaves a dead srv variable; extract the route registration logic used by NewRedisServer into a helper (e.g., buildRedisRoutes(*RedisServer) map[string]handlerFunc) used by both NewRedisServer and this test, then change TestCommand_RouteMatchesTable to iterate the map returned by buildRedisRoutes(srv) and assert each route key exists in redisCommandTable (checking for mismatches both ways), and remove the unused srv declaration and the trailing "_ = srv" no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@adapter/redis_command_info.go`:
- Around line 196-206: Replace the free-form log.Printf call in
warnMissingRedisCommandMeta with structured slog usage: call slog.Warn (or
slog.Logger.Warn) and include the command value as a structured key (e.g.,
command=upper) and a short message about missing redis command metadata; keep
the one-shot guard logic around redisCommandFallbackWarnedOnce and
redisCommandFallbackWarnedOnceMu unchanged. Ensure the file imports slog if not
already present.
In `@adapter/redis_command_test.go`:
- Around line 71-72: The no-op implementations of commandRecorder.WriteRaw and
commandRecorder.WriteAny should not silently drop data; update commandRecorder
to either store a testing.T pointer or otherwise record these invocations so
tests fail or reflect the call: change WriteRaw to append an entry like
{op:"raw", data:...} to the recorder's trace (or call t.Errorf) and change
WriteAny to append {op:"any", value:...} (or similarly report an error) so any
unexpected use is detectable; locate these methods on the commandRecorder type
and modify them to record the operation and payload instead of doing nothing.
- Around line 414-425: TestCommand_RouteMatchesTable currently inspects argsLen
instead of the actual route map and leaves a dead srv variable; extract the
route registration logic used by NewRedisServer into a helper (e.g.,
buildRedisRoutes(*RedisServer) map[string]handlerFunc) used by both
NewRedisServer and this test, then change TestCommand_RouteMatchesTable to
iterate the map returned by buildRedisRoutes(srv) and assert each route key
exists in redisCommandTable (checking for mismatches both ways), and remove the
unused srv declaration and the trailing "_ = srv" no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0da4b01f-ac96-42e2-892b-10a49c20410d
📒 Files selected for processing (5)
adapter/redis.goadapter/redis_command_info.goadapter/redis_command_test.goadapter/redis_compat_commands.gomonitoring/redis.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3632cdc7b2
ℹ️ 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".
| "HDEL": {Name: "hdel", Arity: -3, Flags: []string{redisCmdFlagWrite}, FirstKey: 1, LastKey: 1, Step: 1}, | ||
| "HEXISTS": {Name: "hexists", Arity: 3, Flags: []string{redisCmdFlagReadonly}, FirstKey: 1, LastKey: 1, Step: 1}, |
There was a problem hiding this comment.
redisCommandTable is missing a HELLO row even though HELLO is routed and has an argsLen entry in adapter/redis.go. Because writeCommandInfo looks up only this table, COMMAND INFO HELLO is emitted as nil (unknown command), and COMMAND GETKEYS HELLO ... is treated as invalid despite HELLO being supported. This makes capability probing inconsistent for clients that rely on COMMAND INFO to detect feature support.
Useful? React with 👍 / 👎.
…ble drift Root-cause fix for the post-merge CI failure on this branch (TestCommand _RouteMatchesTable / TestCommandCount_MatchesTableSize) and for the recurring Codex P2 (HELLO missing from redisCommandTable). Both symptoms shared the same underlying cause: command registration was spread across THREE separate maps (r.route in redis.go, argsLen in redis.go, redisCommandTable in redis_command_info.go) and adding a new command required updating all three by hand. HELLO was added to the first two on main but missed the third — silently breaking COMMAND INFO HELLO and tripping the route-vs-table mirror test. This refactor collapses the three views to a single source of truth. Single-row registration (adapter/redis_command_specs.go): - redisCommandSpecs is the canonical slice. Each row carries Constant / Name / Arity / Flags / FirstKey / LastKey / Step. - argsLen is derived from the slice once at init (function-call package var initializer); same for redisCommandTable. - buildRouteMap binds handlers per RedisServer and panics at server construction if any spec lacks a handler or any handler lacks a spec, so the surviving 2-way sync between specs and handlers is enforced at startup rather than at first request. Why handlers stay separate from the spec slice: - Embedding method values directly in the spec literal triggered a Go init cycle (redisCommandSpecs → method body → argsLen → redisCommandSpecs) because Go's init-order analysis follows function bodies. The buildRouteMap layout sidesteps the cycle while keeping the structural invariant under test. Tests added: - TestRedisCommandSpecs_NoDuplicateConstants pins the "every Constant appears exactly once" invariant — without it a duplicate spec would silently shadow earlier rows in the derived maps. - TestRedisCommandSpecs_HELLOPresent is the explicit regression guard: HELLO must have a redisCommandTable row (the exact gap Codex P2 + TestCommand_RouteMatchesTable both flagged). - The pre-existing TestCommand_RouteMatchesTable and TestCommandCount_MatchesTableSize now pass by construction; they remain as the perimeter check for "anything routed must be tabled". Build / vet / lint clean. CI failures from the previous merge commit (3632cdc) are eliminated by construction.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Motivation
COMMANDis issued at connect time by ~every modern Redis client(go-redis, redis-py >= 4, ioredis, node-redis) for capability probing
and key-routing inference. elastickv previously rejected it as
unsupported command, inflating the "unknown" error bucket andbreaking client initialization in strict libraries that actually
consult
COMMAND INFOoutput.Companion to #594 (bounded unsupported-command metric) and #601
(
HELLO). After this +HELLOland, those two commands should coverSubcommand matrix
Implemented:
COMMAND— flat array of per-command info (6-element shape)COMMAND COUNT— integer count of routed commandsCOMMAND LIST— flat array of command namesCOMMAND INFO [name ...]— per-command info array,nilper unknownCOMMAND DOCS [name ...]— minimal map-shaped doc entry per commandCOMMAND GETKEYS <command> <args>— positional key extraction viaper-command first_key/last_key/step
Explicitly rejected (returns
ERR unsupported):COMMAND GETKEYSANDFLAGS— Redis 7 key-specs shape we do not emitCOMMAND LIST FILTERBY MODULE|ACLCAT|PATTERN— elastickv has nomodules and no ACL categories
Metadata table
Single source of truth:
adapter/redis_command_info.go. One row perrouted command holding
(arity, flags, first_key, last_key, step).Adding a new command handler now takes three steps:
RedisServer.route(adapter/redis.go).argsLenentry (adapter/redis.go).redisCommandTableinredis_command_info.go.Forgetting step 3 is caught at CI time by
TestCommand_RouteMatchesTable. In production the runtime fallsthrough to a zero-metadata row and emits a single deduplicated log
warning per missing command name, so the command still appears in
COMMANDoutput (degraded) rather than vanishing entirely.Flags follow a three-value taxonomy:
readonly/write/admin.denyoom/pubsub/staleetc. are intentionally not emitted —no real client consults them for routing decisions.
Wire count, LIST length, and the bare-
COMMANDreply are driven offargsLen(the 1:1 route-keyed set) rather than the table, so thethree subcommands stay mutually consistent even during the brief
window when a new route has been added but the table row is pending.
Test plan
COMMAND COUNTreturnslen(argsLen)and equalslen(redisCommandTable)COMMAND(no args) returns an array of that lengthCOMMAND INFO GET— name/arity/flags/positions match specCOMMAND INFO SET— arity=-3, flags contain "write"COMMAND INFO nosuchcommand— nil entry, not an errorCOMMAND INFO GET NOSUCH SET— 3 entries, middle is nilCOMMAND GETKEYS SET foo bar→["foo"]COMMAND GETKEYS DEL k1 k2 k3→["k1","k2","k3"]redisCommandGetKeysMSET-shape helper test (step=2)COMMAND GETKEYS NOSUCH→ errorCOMMAND LIST— sorted names, length = countCOMMAND LIST FILTERBY MODULE foo— rejectedCOMMAND DOCS GET— 4-element map-shaped entryCOMMAND BADSUB— "Unknown COMMAND subcommand"COMMAND GETKEYSANDFLAGS— rejectedCOMMANDdispatches to the handlerTestCommand_RouteMatchesTable— invariant that every routedcommand has a metadata row
golangci-lint run ./adapter/... ./monitoring/...is cleango test -race -run 'TestCommand|TestRedisCommand' ./adapter/... ./monitoring/...passesSummary by CodeRabbit
Release Notes
COMMANDcommand, enabling client tools and monitoring solutions to query server capabilities. Clients can now probe supported commands and retrieve command metadata including argument requirements, key extraction, and documentation at connect-time.