Use fixed position keys parameter for MSETEX command

[tomerqodo]

Benchmark PR from qodo-benchmark#32

Summary by CodeRabbit

• Chores
  • Updated MSETEX command syntax: now requires an explicit numeric numkeys parameter as the first argument, followed by key-value pairs and optional condition/expiration flags (NX/XX, EX/PX/EXAT/PXAT/KEEPTTL).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The MSETEX command argument structure has been refactored from a keyword-based "KEYS" block approach to use an explicit numeric numkeys parameter followed by a data block. This involves updates to command definitions, JSON schemas, internal parsing logic, and corresponding test cases.

Changes

Cohort / File(s)	Change Summary
Command Definitions src/commands.def, src/commands/msetex.json	MSETEX command spec refactored: key specification changed from keyword-based (KSPEC_BS_KEYWORD) to index-based (KSPEC_BS_INDEX); argument table restructured to replace keys block with explicit numkeys integer argument and data block with multiple entries; arity adjusted from -5 to -4; begin_search migrated from keyword-based to position-based lookup.
String Command Implementation src/t_string.c	Internal argument parsing simplified: removed kv_count and kv_start fields from extendedStringArgs struct; refactored msetexCommand parsing to derive key-value count and indices directly without intermediate KEYS-block bookkeeping; adjusted replication rewriting logic to use derived expiration values instead of positional indices.
Test Suite tests/unit/type/string.tcl	Test invocations updated to reflect new MSETEX syntax with explicit numkeys as first numeric argument; adjusted key/value pair positions; updated error message expectations to match new argument parsing behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 The KEYS have moved from words to numbers true,
Where "numkeys" now leads the data crew.
No keyword block to mark the way,
Just integers that dance and play—
A simpler syntax, fresh and new! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: replacing keyword-based keys parameter with a fixed position parameter in the MSETEX command.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/t_string.c`:
- Around line 718-727: The current guard multiplies kv_count * 2 before
validating, which can overflow; change the validation to check kv_count against
a division-based bound (e.g., if (kv_count > (c->argc - 2) / 2) ) and only
compute the start index (kv_count * 2 + 2) after this check; update the error
path that calls addReplyError to use that new guard and then call
parseExtendedStringArgumentsOrReply(c, kv_count * 2 + 2, &args, COMMAND_MSETEX)
only after validation so parseExtendedStringArgumentsOrReply and subsequent
loops (which index c->argv) cannot run with an overflowed value.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2e2dff and a289297.

📒 Files selected for processing (4)

• src/commands.def
• src/commands/msetex.json
• src/t_string.c
• tests/unit/type/string.tcl

🧰 Additional context used

🧬 Code graph analysis (1)

src/t_string.c (3)

src/object.c (2)

• getRangeLongFromObjectOrReply (1152-1163)
• decrRefCount (594-618)

src/networking.c (2)

• addReplyError (644-647)
• rewriteClientCommandArgument (4424-4477)

src/db.c (1)

• lookupKeyRead (311-313)

🔇 Additional comments (4)

src/commands.def (4)

11143-11145: Key-spec stride matches key/value pairs.

Using FK_KEYNUM with step 2 aligns with the key/value layout after numkeys.

---

11148-11152: Data block subargs are well-formed.

The key/value pair structure is clear and consistent with MSET/MSETNX.

---

11169-11172: Args ordering reflects the fixed-position design.

numkeys followed by the data block cleanly encodes the new schema.

---

11769-11769: Arity update aligns with the new minimum.

-4 matches numkeys + key + value as the minimum argument set.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Prevent kv_count overflow before multiplying.

At Line 718-721 the guard multiplies kv_count * 2 before validating it. On 32‑bit builds, kv_count near INT_MAX can overflow, the check fails, and the loops at Line 738+ can index c->argv out of bounds. Use a division-based bound and compute the start index only after validation.

🛠️ Proposed fix

-    if (kv_count * 2 + 2 > c->argc) {
+    if (kv_count > (c->argc - 2) / 2) {
         addReplyError(c, "wrong number of key-value pairs");
         return;
     }
 
-    extendedStringArgs args;
-    if (parseExtendedStringArgumentsOrReply(c, kv_count * 2 + 2, &args, COMMAND_MSETEX) != C_OK) {
+    int args_start = (int)(kv_count * 2 + 2);
+    extendedStringArgs args;
+    if (parseExtendedStringArgumentsOrReply(c, args_start, &args, COMMAND_MSETEX) != C_OK) {
         return;
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/* Validate we have enough arguments: command + numkeys + (key-value pairs) * 2
	* Be careful to avoid overflow when calculating kv_count * 2 */
	if (kv_count * 2 + 2 > c->argc) {
	addReplyError(c, "wrong number of key-value pairs");
	return;
	}
	/* Validate KEYS block was found */
	if (args.kv_count == 0) {
	addReplyError(c, "syntax error - KEYS keyword is required");
	extendedStringArgs args;
	if (parseExtendedStringArgumentsOrReply(c, kv_count * 2 + 2, &args, COMMAND_MSETEX) != C_OK) {
	return;
	/* Validate we have enough arguments: command + numkeys + (key-value pairs) * 2
	* Be careful to avoid overflow when calculating kv_count * 2 */
	if (kv_count > (c->argc - 2) / 2) {
	addReplyError(c, "wrong number of key-value pairs");
	return;
	}
	int args_start = (int)(kv_count * 2 + 2);
	extendedStringArgs args;
	if (parseExtendedStringArgumentsOrReply(c, args_start, &args, COMMAND_MSETEX) != C_OK) {
	return;

🤖 Prompt for AI Agents

In `@src/t_string.c` around lines 718 - 727, The current guard multiplies kv_count
* 2 before validating, which can overflow; change the validation to check
kv_count against a division-based bound (e.g., if (kv_count > (c->argc - 2) / 2)
) and only compute the start index (kv_count * 2 + 2) after this check; update
the error path that calls addReplyError to use that new guard and then call
parseExtendedStringArgumentsOrReply(c, kv_count * 2 + 2, &args, COMMAND_MSETEX)
only after validation so parseExtendedStringArgumentsOrReply and subsequent
loops (which index c->argv) cannot run with an overflowed value.