feat: add COMMAND DOCS support - #3362
Conversation
![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: f94d60805a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| }, | ||
| transformReply: { | ||
| 2: (reply: UnwrapReply<Resp2Reply<CommandDocsReply>>) => transformDocsMap(reply), |
There was a problem hiding this comment.
When an RESP2 client uses withTypeMapping({ [RESP_TYPES.MAP]: Map }) or maps maps to Array, the public return type is adjusted to that representation, but this transform ignores the supplied typeMapping argument and always converts the top-level reply to a plain object. Code that follows the declared type, such as calling docs.get('get'), therefore fails at runtime; pass the mapping through the recursive transforms and produce the requested representation.
Useful? React with 👍 / 👎.
| deprecated_since?: BlobStringReply; | ||
| replaced_by?: BlobStringReply; | ||
| history?: ArrayReply<TuplesReply<[BlobStringReply, BlobStringReply]>>; | ||
| arguments?: ArrayReply<ReplyUnion>; |
There was a problem hiding this comment.
For TypeScript consumers, each docs.get.arguments entry is currently inferred as the broad ReplyUnion, so ordinary access such as docs.get.arguments?.[0].name is rejected even though transformArgument returns a CommandDocsArgument object and the integration test uses that shape. Type this field as an array of CommandDocsArgument entries, and make the same correction to the recursive CommandDocsArgument.arguments field.
Useful? React with 👍 / 👎.
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
@Hashim1999164 thanks for the PR!
Three points before merge.
First, the RESP2 transformReply should honor the typeMapping parameter (see transformTuplesReply and CONFIG_GET) so users mapping MAP to Map or Array get consistent results across protocols.
Second, please add an integration test running commandDocs against a real Redis 7 server with RESP 2 client options — the nested transform is currently covered only by a synthetic fixture, which is exactly where the previous PR broke.
Third, drop the NOT_KEYED_COMMAND flag, it has no runtime consumer and the other COMMAND family commands do not set it.
Address review feedback: preserve Map/Array type mappings in the RESP2 transform, and type nested arguments as CommandDocsArgument.
|
Addressed the Codex review notes:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd3f52841d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return reply as CommandDoc; | ||
| } | ||
|
|
||
| const doc: Record<string, unknown> = {}; |
There was a problem hiding this comment.
When an RESP2 client uses withTypeMapping({ [RESP_TYPES.MAP]: Map }), the new outer switch returns a Map, but transformDoc still unconditionally creates a plain object and transformArgument does the same. RESP3 applies the mapping recursively in the decoder, so the same command returns Map<string, object> under RESP2 but Map<string, Map> under RESP3; nested maps also fail to honor the requested mapping. Fresh evidence after the earlier review comment is that the added mapping logic only changes transformDocsMap, while this allocation remains unconditional; apply the mapping consistently to document and argument maps or normalize both protocols to the same declared shape.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
@Hashim1999164 Thanks for the update! The typeMapping handling at the top level looks right now, but two of the three requested changes are still outstanding, and one part of the first remains:
- The nested transforms (transformDoc, transformArgument) still produce plain objects unconditionally, so with RESP_TYPES.MAP: Map a RESP2 client gets Map<string, object> while RESP3 returns Map<string, Map>. Please either apply the mapping consistently to the nested maps or normalize both protocols to the same shape.
- Both integration tests use the default RESP3 test setup. Please add a testWithClient run with RESP: 2 client options so the nested RESP2 transform is exercised against a real Redis 7 server — that path is currently only covered by the synthetic fixture.
- NOT_KEYED_COMMAND is still set — please drop it; it has no runtime consumer and the rest of the COMMAND family does not use it.
One smaller thing while you're in there: reply_schema is recursed through transformDoc, which special-cases keys like arguments inside JSON Schema content and only converts one nesting level. Passing the raw value through on RESP2 would be safer.
Happy to take another look once these are in.
|
Addressed the remaining review points: nested RESP2 maps now honor the MAP type mapping, |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 12dbaa1. Configure here.
| } | ||
|
|
||
| export default { | ||
| IS_READ_ONLY: true, |
There was a problem hiding this comment.
Missing NOT_KEYED_COMMAND flag on COMMAND_DOCS
Medium Severity
COMMAND_DOCS is missing NOT_KEYED_COMMAND: true, which every other COMMAND_* sibling declares (COMMAND.ts, COMMAND_COUNT.ts, COMMAND_GETKEYS.ts, COMMAND_GETKEYSANDFLAGS.ts, COMMAND_INFO.ts, COMMAND_LIST.ts). The project's own skill checklist in .agents/skills/implement-command/SKILL.md explicitly requires this flag for commands that take no Redis keys. COMMAND DOCS takes only command names, not keys, so the flag is needed for consistency and correctness in cluster routing contexts.
Reviewed by Cursor Bugbot for commit 12dbaa1. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12dbaa11fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case 'subcommands': | ||
| return transformDocsMap(value, typeMapping); | ||
| case 'reply_schema': | ||
| return value; |
There was a problem hiding this comment.
When a Redis build includes reply_schema in COMMAND DOCS (for example, schema metadata for commands such as GET), RESP2 encodes that schema as nested flattened maps. This branch returns the raw array while the rest of the transformer normalizes maps, so client.commandDocs('GET') under RESP2 yields reply_schema: ['oneOf', ...] and reply_schema.oneOf is undefined even though the RESP3/default shape exposes an object. Recursively transform the schema maps here instead of returning value unchanged.
Useful? React with 👍 / 👎.
| summary?: BlobStringReply; | ||
| since?: BlobStringReply; | ||
| deprecated_since?: BlobStringReply; | ||
| flags?: ArrayReply<BlobStringReply>; |
There was a problem hiding this comment.
When RESP3 callers opt into withTypeMapping({ [RESP_TYPES.SET]: Set }), argument metadata such as flags is decoded as a Set, but this declaration still advertises an array, so TypeScript consumers can write argument.flags?.includes('optional') and then fail at runtime because Set has no includes. Use SetReply<BlobStringReply> for this set-valued field and the analogous doc-level set fields so the public type follows the decoder output.
Useful? React with 👍 / 👎.
| default: | ||
| return value; | ||
| } | ||
| }) as CommandDoc; |
There was a problem hiding this comment.
Fresh evidence after the runtime mapping fix is that this cast still reports each transformed command document as CommandDoc even when callers use withTypeMapping({ [RESP_TYPES.MAP]: Map }) (or Array), where transformMap now returns a Map for every document and the integration test expects get instanceof Map. TypeScript therefore sees docs.get('get') as a plain object and rejects get.get('arguments') even though that is the runtime shape; model command docs/arguments as map replies or provide mapped variants so nested MAP mappings are reflected.
Useful? React with 👍 / 👎.
2 participants
Summary
COMMAND DOCS(client.commandDocs) for Redis 7+, including optional command name filters.argumentsand recursivesubcommands.@since 7.0.0).Fixes #1934
Why previous attempts were not enough
transformArgumentsAPI and did not match the RESP3-first map reply shape maintainers asked for.Test plan
parseArgscovers no args, string, and array filterstransformReply[2]nested map/arguments/subcommandsclient.commandDocs()andclient.commandDocs(['GET', 'SET'])against Redis 7+npm run check:command-jsdocand lint on changed filesNote
Low Risk
Read-only server introspection with localized reply parsing; no auth, data, or connection behavior changes.
Overview
Adds
COMMAND DOCSasclient.commandDocs/COMMAND_DOCS(Redis 7+), with optional variadic command name filters.The implementation builds
COMMAND DOCSviaparseCommand, marks the command read-only, and registers it in the command index with JSDoc (@since 7.0.0). For RESP2, a dedicatedtransformReply[2]turns flat map arrays into the same nested object shape as RESP3—recursivearguments,subcommands, andwithTypeMappingsupport for plain objects vsMap/Array. RESP3 relies on the default map decoder (transformReply[3]is unset).Tests cover argument parsing, RESP2 reply transformation, integration against Redis 7+, filtered queries, and Map type mapping on RESP2.
Reviewed by Cursor Bugbot for commit 12dbaa1. Bugbot is set up for automated code reviews on this repo. Configure here.