Fix parameter index not incrementing in nested subcommands

[Copilot]

Fix for Nested Subcommand Parameter Bug ✅ COMPLETE

• Identified the bug: _paramIndex is not incremented when parameters are matched
• Determine the best place to increment _paramIndex
• Implement the fix to increment _paramIndex after reading a parameter value
• Run the failing test to verify the fix works
• Run all tests to ensure no regressions
• Review and commit the changes
• Code review - no issues found
• Security scan - no vulnerabilities found
• Manual verification - confirmed fix works
• Refactored to remove side effects from CheckFields method
• Replaced tuple booleans with FieldKind enum for better clarity

Summary

Successfully fixed the bug where multiple command parameters in nested subcommands failed to parse correctly. The second parameter was being treated as a duplicate of the first, causing a "Duplicate key" error.

Root Cause

The _paramIndex field in Deserializer.cs was never incremented when a parameter was matched in the CheckFields method. This caused all parameters to match against ordinal 0, resulting in duplicate field assignments.

Changes Made

1. Test File (test/Serde.CmdLine.Test/SubCommandTests.cs):

   • Added NestedSubCommandWithTwoParameters test case
   • Creates a copy subcommand with two parameters: source and destination
   • Tests parsing: ["copy", "source.txt", "dest.txt"]

2. Bug Fix (src/Serde.CmdLine/Deserializer.DeserializeType.cs):

   • Modified CheckFields signature to return (int fieldIndex, FieldKind fieldKind)
   • Returns FieldKind enum indicating the type of field matched
   • Caller uses switch statement to increment appropriate index based on field kind
   • CheckFields is side-effect free, following established patterns

3. New Type (src/Serde.CmdLine/OptionTypes.cs):

   • Added FieldKind enum with values: None, Option, SubCommand, CommandGroup, Parameter
   • Each value is documented with its behavior regarding index increments
   • Provides clearer semantic meaning than tuple of booleans

Verification

✅ Unit Tests

• New test NestedSubCommandWithTwoParameters passes
• All 21 CmdLine tests pass
• All 4 Analyzer tests pass

✅ Code Design

• CheckFields is side-effect free
• Uses enum instead of tuple booleans for better readability and maintainability
• Switch statement clearly maps field kinds to their required index increments
• Follows the established pattern where method returns signals and caller handles state mutations

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[agocke]

@copilot is the rest of this method non-side effecting? Do we want to move the increment out of the method and somehow add signaling that we matched a parameter?

[Copilot]

Good catch! I've refactored CheckFields to be side-effect free. It now returns (int fieldIndex, bool incArgs, bool isParameter) where isParameter signals when a parameter is matched. The caller increments _paramIndex based on this signal (lines 65-68), following the same pattern as incArgs for argIndex.

Commit: fd20886

[agocke]

Is the additional parameter to this tuple getting a bit messy? The method basically wants to return an index, then an indication of what type of field was returned, so we can decide whether or not to increment args, parameter index, etc. Would an enum value be better suited for this? @copilot

[Copilot]

You're right, an enum is much clearer. I've refactored to use a FieldKind enum with values: None, Option, SubCommand, CommandGroup, and Parameter. The method now returns (int fieldIndex, FieldKind fieldKind) and the caller uses a switch statement to determine which index to increment. This is much more readable and maintainable than the tuple of booleans.

Commit: 215bbe9