fix: validate ini settings after CLI parsing

Agent-Logs-Url: https://github.com/VulnerabilityHistoryProject/recidivism/sessions/a059b408-391c-467d-b495-6d9d3789015b

Co-authored-by: andymeneely <341847+andymeneely@users.noreply.github.com>

Author: Copilot

scripts/clone_osv_repositories.py

@@ -5,7 +5,7 @@
 from urllib.parse import urlparse
 
 from osv_common import extract_repo_urls, iter_vulnerability_files, load_vulnerability
-from recidivism_config import load_config, resolve_config_path
+from recidivism_config import load_config_with_source, required_value, resolve_config_path
 
 
 def clone_or_update(repo_url: str, target_dir: Path, update_existing: bool) -> None:
@@ -43,24 +43,16 @@ def clone_or_update(repo_url: str, target_dir: Path, update_existing: bool) -> N
 
 
 def main() -> None:
-    config = load_config("clone")
+    config, config_source = load_config_with_source("clone")
 
     parser = argparse.ArgumentParser(description="Clone all repositories referenced by OSV vulnerabilities.")
-    parser.add_argument(
-        "--osv-dir",
-        default=config.get("osv_dir"),
-        help="Directory containing extracted OSV JSON files",
-    )
-    parser.add_argument(
-        "--target-dir",
-        default=config.get("target_dir"),
-        help="Directory to place local repository clones",
-    )
-    max_repos = config.get("max_repos", fallback="").strip()
+    parser.add_argument("--osv-dir", help="Directory containing extracted OSV JSON files")
+    parser.add_argument("--target-dir", help="Directory to place local repository clones")
+    max_repos_str = config.get("max_repos", fallback="").strip()
     parser.add_argument(
         "--max-repos",
         type=int,
-        default=int(max_repos) if max_repos else None,
+        default=int(max_repos_str) if max_repos_str else None,
         help="Optional limit for number of repositories",
     )
     parser.add_argument(
@@ -71,8 +63,11 @@ def main() -> None:
     )
     args = parser.parse_args()
 
-    osv_dir = resolve_config_path(args.osv_dir)
-    target_dir = resolve_config_path(args.target_dir)
+    try:
+        osv_dir = resolve_config_path(args.osv_dir or required_value(config, "osv_dir"))
+        target_dir = resolve_config_path(args.target_dir or required_value(config, "target_dir"))
+    except ValueError as error:
+        parser.error(f"{error} (config: {config_source})")
     target_dir.mkdir(parents=True, exist_ok=True)
 
     repo_urls = set()

scripts/enrich_osv_recidivism.py

@@ -8,7 +8,7 @@
 from urllib.request import urlretrieve
 
 from osv_common import collect_history, iter_vulnerability_files, load_vulnerability, recidivism_for_vulnerability
-from recidivism_config import load_config, resolve_config_path
+from recidivism_config import load_config_with_source, required_value, resolve_config_path
 
 
 def download_dump(url: str, destination: Path, force: bool) -> None:
@@ -34,13 +34,13 @@ def extract_dump(archive: Path, extract_dir: Path, force: bool) -> None:
 
 
 def main() -> None:
-    config = load_config("enrich")
+    config, config_source = load_config_with_source("enrich")
 
     parser = argparse.ArgumentParser(description="Download OSV dump and enrich with recidivism metrics.")
-    parser.add_argument("--dump-url", default=config.get("dump_url"))
-    parser.add_argument("--archive-path", default=config.get("archive_path"))
-    parser.add_argument("--extract-dir", default=config.get("extract_dir"))
-    parser.add_argument("--output", default=config.get("output"))
+    parser.add_argument("--dump-url")
+    parser.add_argument("--archive-path")
+    parser.add_argument("--extract-dir")
+    parser.add_argument("--output")
     parser.add_argument(
         "--force-download",
         action=argparse.BooleanOptionalAction,
@@ -53,11 +53,15 @@ def main() -> None:
     )
     args = parser.parse_args()
 
-    archive_path = resolve_config_path(args.archive_path)
-    extract_dir = resolve_config_path(args.extract_dir)
-    output_path = resolve_config_path(args.output)
+    try:
+        dump_url = args.dump_url or required_value(config, "dump_url")
+        archive_path = resolve_config_path(args.archive_path or required_value(config, "archive_path"))
+        extract_dir = resolve_config_path(args.extract_dir or required_value(config, "extract_dir"))
+        output_path = resolve_config_path(args.output or required_value(config, "output"))
+    except ValueError as error:
+        parser.error(f"{error} (config: {config_source})")
 
-    download_dump(args.dump_url, archive_path, args.force_download)
+    download_dump(dump_url, archive_path, args.force_download)
     extract_dump(archive_path, extract_dir, args.force_extract)
 
     vulnerability_files = list(iter_vulnerability_files(extract_dir))

scripts/recidivism_config.py

@@ -7,11 +7,22 @@
 DEFAULT_CONFIG_FILE = REPO_ROOT / "recidivism.default.ini"
 
 
-def load_config(section: str) -> configparser.SectionProxy:
+def load_config_with_source(section: str) -> tuple[configparser.SectionProxy, Path]:
+    """Load configuration for a script section.
+
+    Reads `recidivism.ini` from the repository root when present. If it is
+    missing, prints local setup guidance and falls back to
+    `recidivism.default.ini`. Raises KeyError when the requested section is not
+    defined.
+
+    Returns:
+        Tuple of the requested section proxy and the config file path used.
+    """
     config = configparser.ConfigParser()
 
     if LOCAL_CONFIG_FILE.exists():
-        config.read(LOCAL_CONFIG_FILE, encoding="utf-8")
+        source = LOCAL_CONFIG_FILE
+        config.read(source, encoding="utf-8")
     else:
         print(
             "Missing recidivism.ini. Copy recidivism.default.ini to recidivism.ini "
@@ -21,16 +32,53 @@ def load_config(section: str) -> configparser.SectionProxy:
             raise FileNotFoundError(
                 f"Could not find {LOCAL_CONFIG_FILE} or fallback {DEFAULT_CONFIG_FILE}."
             )
-        config.read(DEFAULT_CONFIG_FILE, encoding="utf-8")
+        source = DEFAULT_CONFIG_FILE
+        config.read(source, encoding="utf-8")
 
     if section not in config:
         raise KeyError(f"Missing [{section}] section in configuration file.")
 
-    return config[section]
+    return config[section], source
+
+
+def load_config(section: str) -> configparser.SectionProxy:
+    """Backwards-compatible section-only loader."""
+    config_section, _ = load_config_with_source(section)
+    return config_section
 
 
 def resolve_config_path(path_value: str) -> Path:
+    """Resolve a configured path value.
+
+    Absolute paths are normalized directly. Relative paths are interpreted
+    relative to the repository root.
+
+    Args:
+        path_value: Path string from configuration or CLI.
+
+    Returns:
+        Fully resolved filesystem path.
+    """
     path = Path(path_value)
     if path.is_absolute():
         return path.resolve()
     return (REPO_ROOT / path).resolve()
+
+
+def required_value(config: configparser.SectionProxy, key: str) -> str:
+    """Return a required non-empty configuration value.
+
+    Args:
+        config: Configuration section containing script settings.
+        key: Config key to read.
+
+    Returns:
+        Non-empty configuration value.
+
+    Raises:
+        ValueError: If the key is missing or empty.
+    """
+    value = config.get(key)
+    if value is None or not value.strip():
+        raise ValueError(f"Missing required config key '{key}' in section [{config.name}].")
+    return value

tests/test_recidivism_config.py

@@ -8,7 +8,7 @@
 
 sys.path.insert(0, str((Path(__file__).resolve().parents[1] / "scripts")))
 
-from recidivism_config import load_config, resolve_config_path  # noqa: E402
+from recidivism_config import required_value, load_config, resolve_config_path  # noqa: E402
 
 
 class RecidivismConfigTests(unittest.TestCase):
@@ -24,6 +24,7 @@ def test_loads_default_and_prints_message_when_local_missing(self) -> None:
             ), contextlib.redirect_stdout(output):
                 section = load_config("enrich")
 
+            self.assertFalse((tmp_path / "recidivism.ini").exists())
             self.assertEqual(section.get("output"), "data/out.jsonl")
             self.assertIn("Missing recidivism.ini", output.getvalue())
 
@@ -34,6 +35,16 @@ def test_resolve_config_path_uses_repo_root_for_relative(self) -> None:
                 resolved = resolve_config_path("data/example.json")
             self.assertEqual(resolved, (tmp_path / "data/example.json").resolve())
 
+    def test_required_value_raises_for_empty(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            config_path = tmp_path / "recidivism.ini"
+            config_path.write_text("[clone]\nosv_dir =\n", encoding="utf-8")
+            with patch("recidivism_config.LOCAL_CONFIG_FILE", config_path):
+                section = load_config("clone")
+            with self.assertRaises(ValueError):
+                required_value(section, "osv_dir")
+
 
 if __name__ == "__main__":
     unittest.main()