Skip to content

Conversation

@avionicharshit-byte
Copy link
Contributor

Summary

This PR adds strict integer parsing for CLI arguments to prevent silent acceptance of malformed input. Previously, the CLI used std::stoi/stoul/stoull which accepts trailing characters (e.g., "123abc" was silently parsed as 123). This fix ensures that invalid inputs are properly rejected with clear error messages.

Fixes #225

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

New Utility Functions (cli/utils/util.h/cpp)

  • parseStrictInt() - Strict integer parsing with trailing character rejection
  • parseStrictULong() - Strict unsigned long parsing
  • parseStrictULL() - Strict unsigned long long parsing

All functions throw InvalidArgsException with descriptive error messages when:

  • Input string is empty
  • Input contains trailing non-numeric characters
  • Input cannot be parsed

Replacements

  1. cli/args/GlobalArgs.h - verbosity parameter parsing
  2. cli/args/BenchmarkArgs.h - level and numIters parameters (2 instances)
  3. cli/utils/compress_profiles.cpp - chunkSize parameter parsing
  4. cli/args/TrainArgs.h - threads, numSamples, maxTimeSecs, maxFileSizeMb, maxTotalSizeMb (5 instances)
  5. cli/args/CompressArgs.h - trainInlineTestLimit parameter parsing

Architecture Changes

  • Moved ProfileArgs constructor from header to .cpp to avoid circular dependencies
  • Added documentation comment explaining design decision for multiple parsing functions

Test Plan

Manual Testing

# Valid input - works correctly
./build/cli/zli --verbose 3 list-profiles
✅ Output: Lists all available profiles

# Invalid input - properly rejected (FIX FOR ISSUE #225)
./build/cli/zli --verbose 3abc list-profiles
✅ Output: Invalid argument(s):
         Invalid integer: '3abc' contains trailing characters starting at position 1

Automated Testing

- Created comprehensive unit test suite: tests/unittest/common/test_strict_parsing.cpp
- 10 tests covering:
  - Valid input acceptance for all three functions
  - Trailing character rejection (core fix for #225)
  - Invalid input handling (empty strings, etc.)
  - Explicit regression test for issue #225

Test Results:
./build/tests/unittest --gtest_filter="StrictParsingTest.*"
[  PASSED  ] 10 tests.

Test Configuration

- Compiler: Apple clang (ARM64)
- Build type: Release
- Platform: macOS (Apple Silicon)
- CMake:  3.31.6
- Test framework: Google Test

Verification

Before fix:
# Silently accepted invalid input
zli --verbose 123abc list-profiles  # Treated as --verbose 123

After fix:
# Properly rejects invalid input
zli --verbose 123abc list-profiles
# Error: Invalid integer: '123abc' contains trailing characters starting at position 3

Files Modified

- cli/utils/util.h (+26 lines)
- cli/utils/util.cpp (+79 lines)
- cli/args/GlobalArgs.h (1 replacement)
- cli/args/BenchmarkArgs.h (2 replacements)
- cli/args/TrainArgs.h (5 replacements)
- cli/args/CompressArgs.h (1 replacement)
- cli/utils/compress_profiles.h (constructor moved to .cpp)
- cli/utils/compress_profiles.cpp (+17 lines, constructor implementation)
- tests/unittest/common/test_strict_parsing.cpp (+76 lines, NEW)
- tests/CMakeLists.txt (+1 line, linking utils and args for CLI tests)

@meta-cla meta-cla bot added the cla signed label Dec 5, 2025
@avionicharshit-byte avionicharshit-byte changed the title Fix/strict-integer-parsing fix/strict-integer-parsing Dec 5, 2025
@avionicharshit-byte avionicharshit-byte marked this pull request as draft December 5, 2025 18:37
@avionicharshit-byte avionicharshit-byte force-pushed the fix/issue-225-strict-integer-parsing branch from 858ab7c to 274b883 Compare December 5, 2025 19:04
@avionicharshit-byte avionicharshit-byte force-pushed the fix/issue-225-strict-integer-parsing branch from 93169a2 to 9570e9a Compare December 5, 2025 19:31
@avionicharshit-byte avionicharshit-byte marked this pull request as ready for review December 5, 2025 19:33
@avionicharshit-byte
Copy link
Contributor Author

@Cyan4973 could you please review it? also, the third commit is to fix the build failure, i am open to any suggestions or changes on that

@avionicharshit-byte
Copy link
Contributor Author

avionicharshit-byte commented Dec 11, 2025

@terrelln can you review this pr ?

@avionicharshit-byte
Copy link
Contributor Author

avionicharshit-byte commented Dec 23, 2025

@Cyan4973 any updates on this pr?

@avionicharshit-byte
Copy link
Contributor Author

@felixhandte can you review this pr ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI integer parsing accepts trailing characters

1 participant