chore(server): migrate main_service handlers to CmdArgParser

[romange]

Summary

Migrate most main_service.cc command handlers from CmdArgList to CmdArgParser, following the same pattern established in the recent set_family.cc and zset_family.cc migrations.

Changes

• Migrate handlers: Quit, Multi, Watch, Unwatch, Discard, Exec, Publish, Subscribe, Unsubscribe, PSubscribe, PUnsubscribe, Function, Monitor, Pubsub, Command
• Propagate ParsedArgs through ChangeSubscription/ChangePSubscription/ChangeSubscriptions in conn_context to avoid CmdArgList conversion at call sites
• Update MFUNC macro to call MakeParserFromContext instead of forwarding CmdArgList

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Migrate main_service command handlers to CmdArgParser
✨ Enhancement 🕐 20-40 Minutes

Description

• Migrate main_service command handlers from CmdArgList to CmdArgParser.
• Thread ParsedArgs through pubsub subscription helpers to avoid per-call conversions.
• Update MFUNC to build parsers from context; keep MFUNC_OLD for Eval* follow-up.

Diagram

graph TD
  A["CommandRegistry"] --> B["MFUNC macro"] --> C["MakeParserFromContext"] --> D["CmdArgParser"] --> E["main_service handlers"] --> F["ConnectionContext pubsub"]
  F --> G["ChangeSubscriptions(ParsedArgs)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep CmdArgList at the Service boundary (adapter-only migration)

• ➕ Minimizes signature churn across Service and ConnectionContext.
• ➕ Reduces risk of subtle argument iteration differences.
• ➖ Continues paying conversion costs and duplicated parsing patterns.
• ➖ Delays convergence to a single handler API.

2. Complete migration including Eval* in the same PR

• ➕ Eliminates MFUNC_OLD and reduces mixed handler styles.
• ➕ Finishes the transition in one review cycle.
• ➖ Increases blast radius and review complexity (scripting path is riskier).
• ➖ Harder to bisect if regressions appear in eval-related commands.

3. Introduce a dual-mode handler wrapper (templated wrapper over CmdArgList/CmdArgParser)

• ➕ Centralizes compatibility glue without per-command macros.
• ➕ Can reduce macro complexity over time.
• ➖ Adds template/macro abstraction that may be harder to debug.
• ➖ Still requires a staged migration plan and careful API design.

Recommendation: Proceed with the current staged migration: moving the bulk of main_service to CmdArgParser and threading ParsedArgs into ConnectionContext reduces conversion overhead and unifies handler style while keeping Eval* deferred to limit risk. A follow-up PR to migrate Eval* and remove MFUNC_OLD should be planned to avoid a long-lived split API.

Files changed (4) +77 / -76

Refactor (4) +77 / -76

conn_context.ccAccept ParsedArgs in subscription helpers and response formatting +7/-5

Accept ParsedArgs in subscription helpers and response formatting

• Updates ChangeSubscription/ChangePSubscription to accept facade::ParsedArgs and uses direct args indexing when emitting subscription change responses. Converts ChangeSubscriptions to operate on ParsedArgs instead of CmdArgList.

src/server/conn_context.cc

conn_context.hChange ConnectionContext subscription method signatures to ParsedArgs +3/-3

Change ConnectionContext subscription method signatures to ParsedArgs

• Adjusts public/private ConnectionContext method declarations to accept facade::ParsedArgs (by const ref / by value) for subscription operations. Aligns header signatures with implementation to remove CmdArgList from pubsub subscription plumbing.

src/server/conn_context.h

main_service.ccMigrate command handlers to CmdArgParser and update MFUNC binding +50/-51

Migrate command handlers to CmdArgParser and update MFUNC binding

• Switches many handlers (e.g., Quit/Multi/Watch, pubsub commands, Command) from CmdArgList to CmdArgParser and updates argument access to Next()/HasNext()/UnparsedArgs(). Updates MFUNC to build a parser via MakeParserFromContext, and introduces MFUNC_OLD for Eval* handlers pending migration.

src/server/main_service.cc

main_service.hUpdate Service handler declarations to CmdArgParser/ParsedArgs +17/-17

Update Service handler declarations to CmdArgParser/ParsedArgs

• Updates handler declarations to take facade::CmdArgParser and adjusts PubsubNumSub to take facade::ParsedArgs. Keeps Eval* handlers on CmdArgList consistent with MFUNC_OLD usage.

src/server/main_service.h

[augmentcode]

🤖 Augment PR Summary

Summary: This PR migrates a large set of main_service.cc command handlers from CmdArgList-based argument access to facade::CmdArgParser.

Changes:

• Converted multiple core commands (transactions, pub/sub, monitoring, command introspection) to use `CmdArgParser` APIs like `Next()`/`HasNext()`/`UnparsedArgs()`.
• Propagated `facade::ParsedArgs` through `ConnectionContext::ChangeSubscription` / `ChangePSubscription` and `ChangeSubscriptions` to avoid converting back to `CmdArgList` at call sites.
• Updated `PubsubNumSub` to accept `ParsedArgs` directly.
• Reworked the `MFUNC` macro to build parsers from `CommandContext` via `MakeParserFromContext`.
• Kept the legacy `MFUNC_OLD` path for `Eval*` handlers to be migrated in a follow-up.

Technical Notes: The new flow relies on CommandContext::tail_args() as the canonical source of “arguments without the command name”, enabling parser-based handlers without needing CmdArgList plumbing.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

src/server/main_service.cc:2762 (COMMAND INFO): after consuming the command name via parser.Next(), any additional arguments are silently ignored because there’s no validation of leftover args. This changes behavior vs the old args.size() == 2 check and could hide client misuse (or should intentionally support multiple command names).

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. INFO drops trailing args 🐞 Bug ≡ Correctness

Description

Service::Command now handles COMMAND INFO by consuming only one command-name and returning
without validating that no additional args remain, so inputs like COMMAND INFO GET SET silently
ignore SET. This is a behavior regression introduced by replacing the prior exact-arity check with
parser.HasNext() and not calling Finalize() (which is the only mechanism that reports
UNPROCESSED).

Code

src/server/main_service.cc[R2762-2766]

+  if (subcmd == "INFO" && parser.HasNext()) {
+    string cmd = absl::AsciiStrToUpper(parser.Next());

   if (const auto* cid = registry_.Find(cmd); cid) {
     rb->StartArray(1);

Evidence

The COMMAND INFO branch consumes exactly one arg and returns, leaving any remaining args
unvalidated. CmdArgParser only reports leftover args via Finalize() (by setting UNPROCESSED),
and its destructor does not enforce that all args were consumed, so the trailing args are ignored
without error.

src/server/main_service.cc[2705-2795]
src/facade/cmd_arg_parser.h[273-289]
src/facade/cmd_arg_parser.cc[73-76]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Service::Command` parses `COMMAND INFO` by reading a single command name (`parser.Next()`) and immediately returning. Any remaining args are silently ignored because the handler does not validate leftovers (and `CmdArgParser` does not error on trailing args unless `Finalize()` is called).
### Issue Context
The help text in `COMMAND HELP` documents `INFO command-name` (singular). If callers pass multiple command names, the server currently responds with info for only the first, which is surprising and inconsistent.
### Fix Focus Areas
- src/server/main_service.cc[2761-2773]
### Suggested fix
After consuming the command name for `INFO`, either:
1) Enforce strict arity: if `parser.HasNext()` is still true, return a syntax/wrong-arity error; or
2) Implement multi-name support: iterate over all remaining args, returning an array of N entries (one per requested command), using `serialize_command` / null per miss.
If you choose strict arity, you can also call `parser.Finalize()` after consuming expected args and return `parser.TakeError().MakeReply()` on failure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[BorysTheDev]

parser.Next process this situation

[BorysTheDev]

feels like you need Map or Apply

[romange]

I prefer not to be blocked on it right now