fix(version-scanner): address reviewer feedback regarding encoding, float versions, and argument validation

Author: chalmerlowe

scripts/version_scanner/tests/unit/test_version_scanner.py

@@ -59,6 +59,8 @@ def sample_match():
         (PermissionError(), False, False, False, "Warning: Permission denied reading test_desc", None),  # Optional PermissionError
         (IOError("disk full"), True, False, True, "Error reading test_desc", None),  # Required IOError
         (IOError("disk full"), False, False, False, "Warning: Error reading test_desc", None),  # Optional IOError
+        (ValueError("invalid bytes"), True, False, True, "Error reading test_desc", None),  # Required ValueError
+        (ValueError("invalid bytes"), False, False, False, "Warning: Error reading test_desc", None),  # Optional ValueError
     ]
 )
 def test_safe_read_file_scenarios(
@@ -801,6 +803,8 @@ def test_parse_matrix_file(tmp_path):
         ("invalid: {", True),           # Invalid YAML
         ("- not_a_mapping", True),      # Invalid structure (list instead of map)
         ("python:\n  - null", True),    # Invalid version type (null/None value)
+        ("python:\n  - 3.10", True),    # Invalid version type (float instead of string in list)
+        ("python: 3.10", True),         # Invalid version type (float instead of string)
     ]
 )
 def test_parse_matrix_file_failures(tmp_path, file_content, file_exists):
@@ -868,3 +872,26 @@ def test_scan_repository_multi_targets(tmp_path):
     assert protobuf_match[0]["version"] == "4.25.8"
     assert protobuf_match[0]["rule_name"] == "protobuf_check"
 
+
+@pytest.mark.parametrize(
+    "args, expected_error_msg",
+    [
+        # Mixing -m/--matrix-file with -d or -v
+        (['version_scanner.py', '-m', 'matrix.yaml', '-d', 'python'], "Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file"),
+        (['version_scanner.py', '-m', 'matrix.yaml', '-v', '3.7'], "Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file"),
+        (['version_scanner.py', '-m', 'matrix.yaml', '-d', 'python', '-v', '3.7'], "Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file"),
+        # Missing either -d or -v when not using -m
+        (['version_scanner.py', '-d', 'python'], "Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file"),
+        (['version_scanner.py', '-v', '3.7'], "Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file"),
+        (['version_scanner.py'], "Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file"),
+    ]
+)
+def test_main_cli_validation(capsys, args, expected_error_msg):
+    from version_scanner import main
+    with mock.patch('sys.argv', args):
+        with pytest.raises(SystemExit) as excinfo:
+            main()
+        assert excinfo.value.code == 2
+        captured = capsys.readouterr()
+        assert expected_error_msg in captured.err
+

scripts/version_scanner/version_scanner.py

@@ -65,7 +65,7 @@ def _safe_read_file(
         else:
             print(f"Warning: Permission denied reading {description}: {file_path}", file=sys.stderr)
             return None
-    except IOError as e:
+    except (IOError, ValueError) as e:
         if required:
             print(f"Error reading {description} {file_path}: {e}", file=sys.stderr)
             sys.exit(1)
@@ -646,11 +646,14 @@ def parse_matrix_file(file_path: str) -> List[Tuple[str, str]]:
                 if v is None or isinstance(v, (dict, list)):
                     print(f"Error: Invalid version '{v}' for dependency '{dep}'", file=sys.stderr)
                     sys.exit(1)
-                targets.append((str(dep), str(v)))
-        elif versions is not None and not isinstance(versions, dict):
-            targets.append((str(dep), str(versions)))
+                if not isinstance(v, str):
+                    print(f"Error: Version '{v}' for dependency '{dep}' must be specified as a quoted string to prevent YAML parsing issues (e.g., 3.10 parsed as 3.1).", file=sys.stderr)
+                    sys.exit(1)
+                targets.append((str(dep), v))
+        elif isinstance(versions, str):
+            targets.append((str(dep), versions))
         else:
-            print(f"Error: Invalid version '{versions}' for dependency '{dep}'", file=sys.stderr)
+            print(f"Error: Invalid version '{versions}' for dependency '{dep}'. Versions must be specified as quoted strings.", file=sys.stderr)
             sys.exit(1)
 
     return targets
@@ -743,13 +746,13 @@ def main():
     args = parser.parse_args()
 
     # Validation of required inputs
-    has_single_target = bool(args.dependency and args.version)
     has_matrix_file = bool(args.matrix_file)
-    
-    if not (has_single_target or has_matrix_file):
-        parser.error("Must specify either (-d/--dependency AND -v/--version) OR (-m/--matrix-file)")
-    if has_single_target and has_matrix_file:
-        parser.error("Cannot specify both single target (-d/-v) and matrix file (-m/--matrix-file)")
+    if has_matrix_file:
+        if args.dependency or args.version:
+            parser.error("Cannot specify -d/--dependency or -v/--version when using -m/--matrix-file")
+    else:
+        if not (args.dependency and args.version):
+            parser.error("Must specify both -d/--dependency and -v/--version when not using -m/--matrix-file")
 
     targets = []
     if has_matrix_file: