feat: support multiple -p/-r flags with comma separation

[yimsk]

Summary

• Support multiple profiles/regions via comma separation (-p dev,prod) or repeated flags (-p dev -p prod)
• Add comprehensive tests for flag parsing (130 lines)
• Update docs, README, and demo GIF

[yimsk]

Fixed test isolation: extracted parseFlagsFromArgs() to avoid os.Args mutation.

[claude]

Code Review: PR #111 - Multiple Profiles/Regions Support

Summary

This PR adds support for multiple profiles and regions via comma-separated values or repeated flags. The implementation is well-structured with comprehensive test coverage. Overall, this is a solid implementation that follows the codebase patterns well.

✅ Strengths

1. Excellent Test Coverage

• 126 lines of well-structured table-driven tests
• Tests all edge cases: comma separation, repeated flags, mixed usage, empty values, duplicates, whitespace trimming
• Follows existing test patterns perfectly (see internal/config/config_test.go)
• Added tests are isolated and testable via parseFlagsFromArgs(args []string)

2. Security - Input Validation

• Proper validation using existing config.IsValidProfileName() and config.IsValidRegion()
• Iterates through all profiles/regions with individual validation (main.go:44-57)
• No shell injection risk - values are validated before use
• Character whitelisting via regex patterns (not blacklisting)

3. Code Quality

• Follows existing patterns for multi-value config (consistent with SetSelections(), SetRegions())
• Proper deduplication using slices.Contains()
• Whitespace trimming and empty value filtering
• Clear error messages with guidance for users
• Maintains backward compatibility with single values

4. Documentation

• Updated README with clear examples
• Updated configuration.md with usage notes
• Updated demo to showcase the feature
• Updated usage/help text appropriately

🔍 Observations & Minor Suggestions

1. Order Preservation

The current implementation preserves insertion order while removing duplicates:

if p = strings.TrimSpace(p); p != "" && !slices.Contains(opts.profiles, p) {
    opts.profiles = append(opts.profiles, p)
}

Note: slices.Contains() is O(n) per insertion, making the overall parsing O(n²). For typical use cases with 2-10 profiles, this is negligible. If you anticipate users with dozens of profiles, consider using a map for deduplication, but this is not a blocker.

2. Consistent Logging

✅ Already addressed: Updated logging to use plural forms (main.go:87):

log.Info("claws started", "profiles", opts.profiles, "regions", opts.regions, "readOnly", opts.readOnly)

3. Empty Flag Value Handling

Current behavior when flag is provided without value:

case "-p", "--profile":
    if i+1 < len(args) {  // No error if missing value

This silently ignores -p at end of args. Consider if you want to show an error:

if i+1 >= len(args) {
    fmt.Fprintln(os.Stderr, "Error: -p/--profile requires a value")
    os.Exit(1)
}

However, checking other flags in the codebase, this appears to be consistent with existing behavior (same pattern for -r, -s, etc.), so this is acceptable.

4. Test Coverage - Integration

The unit tests are excellent. Consider adding an integration test that verifies:

• applyStartupConfig() correctly converts opts.profiles to config.ProfileSelection slices
• CLI flags override config file settings with multiple values

Example test structure:

func TestApplyStartupConfig_MultipleProfiles(t *testing.T) {
    opts := cliOptions{
        profiles: []string{"dev", "prod"},
        regions:  []string{"us-east-1", "eu-west-1"},
    }
    cfg := config.NewConfig()
    fileCfg := &config.FileConfig{}
    
    applyStartupConfig(opts, fileCfg, cfg)
    
    if !cfg.IsMultiProfile() {
        t.Error("Expected multi-profile to be enabled")
    }
    // ... verify selections
}

Not blocking, but would increase confidence in the integration between flag parsing and config application.

🎯 Potential Issues

1. Missing Test for Combined Flags (Minor)

The test TestParseFlags_Combined() only verifies that values are parsed, but doesn't verify interaction with other flags like -s, --autosave, etc. Consider adding a test with many flags combined.

Example:

func TestParseFlags_AllFlags(t *testing.T) {
    opts := parseFlagsFromArgs([]string{
        "-p", "dev,prod",
        "-r", "us-east-1,eu-west-1", 
        "-s", "ec2",
        "-i", "i-12345",
        "-ro",
        "--autosave",
    })
    // Verify all flags parsed correctly
}

Impact: Low - unlikely to have issues, but improves coverage

2. Help Text Formatting (Very Minor)

The usage examples in printUsage() (main.go:236-238) could be more aligned:

// Before:
fmt.Println("  claws -p dev,prod                 Query multiple profiles")
fmt.Println("  claws -r us-east-1,ap-northeast-1 Query multiple regions")

// Suggestion - align descriptions:
fmt.Println("  claws -p dev,prod                     Query multiple profiles")
fmt.Println("  claws -r us-east-1,ap-northeast-1     Query multiple regions")

Impact: Cosmetic only

📋 Performance Considerations

✅ No concerns:

• Parsing is O(n²) due to slices.Contains(), but n is typically < 10
• No memory allocations beyond necessary slice growth
• Validation regex patterns are pre-compiled
• Thread-safe config access uses RWMutex appropriately

🔒 Security Considerations

✅ No concerns:

• All inputs validated against strict regex patterns before use
• Length limits enforced (region: 25 chars, profile: 128 chars)
• No command injection vectors
• Character whitelisting prevents injection attacks
• Values used directly in AWS SDK calls (no shell execution)

📊 Test Coverage Summary

Test Category	Coverage	Quality
Flag parsing - profiles	✅ Excellent	8 test cases covering all edge cases
Flag parsing - regions	✅ Excellent	5 test cases
Combined flags	✅ Good	Basic combination tested
Edge cases	✅ Excellent	Empty, duplicates, whitespace
Integration	⚠️ Missing	Config application not unit tested

🎯 Final Recommendation

Approve with minor suggestions. This is high-quality code that:

• Follows existing patterns
• Has excellent test coverage
• Properly validates inputs
• Maintains backward compatibility
• Includes good documentation

The suggestions above are optional improvements, not blocking issues.

📝 Optional Follow-ups (Non-blocking)

1. Add integration test for applyStartupConfig() with multiple values
2. Consider adding explicit error for missing flag values
3. Add test for all flags combined
4. Minor help text alignment

Great work! 🎉