fix: address code review feedback from Copilot

- Use collections.abc.Mapping instead of dict for cooldown type check
  to support ruamel.yaml CommentedMap
- Use ordered tuple for cooldown days keys to ensure deterministic
  YAML output order
- Fix make_dependabot_config return type annotation (-> None) since
  it mutates in place and callers ignore the return value
- Remove dead yaml.dump call from make_dependabot_config
- Update test for extra config YAML error to test at loading stage

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: zkoppert

dependabot_file.py

@@ -3,6 +3,7 @@
 import base64
 import copy
 import io
+from collections.abc import Mapping
 
 import github3
 import ruamel.yaml
@@ -19,9 +20,13 @@
 stream = io.StringIO()
 
 
-VALID_COOLDOWN_DAYS_KEYS = frozenset(
-    {"default-days", "semver-major-days", "semver-minor-days", "semver-patch-days"}
+COOLDOWN_DAYS_KEYS_ORDERED = (
+    "default-days",
+    "semver-major-days",
+    "semver-minor-days",
+    "semver-patch-days",
 )
+VALID_COOLDOWN_DAYS_KEYS = frozenset(COOLDOWN_DAYS_KEYS_ORDERED)
 VALID_COOLDOWN_KEYS = VALID_COOLDOWN_DAYS_KEYS | {"include", "exclude"}
 MAX_COOLDOWN_LIST_ITEMS = 150
 MIN_COOLDOWN_DAYS = 1
@@ -38,7 +43,7 @@ def validate_cooldown_config(cooldown):
     Raises:
         ValueError: if the cooldown configuration is invalid
     """
-    if not isinstance(cooldown, dict):
+    if not isinstance(cooldown, Mapping):
         raise ValueError("Cooldown configuration must be a mapping")
 
     unknown_keys = set(cooldown.keys()) - VALID_COOLDOWN_KEYS
@@ -93,9 +98,11 @@ def make_dependabot_config(
     dependabot_config,
     extra_dependabot_config,
     cooldown=None,
-) -> str:
+) -> None:
     """
-    Make the dependabot configuration for a specific package ecosystem
+    Make the dependabot configuration for a specific package ecosystem.
+
+    Mutates dependabot_config in place by appending an update entry.
 
     Args:
         ecosystem: the package ecosystem to make the dependabot configuration for
@@ -106,9 +113,6 @@ def make_dependabot_config(
         dependabot_config: extra dependabot configs
         extra_dependabot_config: File with the configuration to add dependabot configs (ex: private registries)
         cooldown: optional cooldown configuration dict to delay version update PRs
-
-    Returns:
-        str: the dependabot configuration for the package ecosystem
     """
 
     dependabot_config["updates"].append(
@@ -166,7 +170,7 @@ def make_dependabot_config(
 
     if cooldown:
         cooldown_config = {}
-        for key in VALID_COOLDOWN_DAYS_KEYS:
+        for key in COOLDOWN_DAYS_KEYS_ORDERED:
             if key in cooldown:
                 cooldown_config[key] = cooldown[key]
         for list_key in ("include", "exclude"):
@@ -176,8 +180,6 @@ def make_dependabot_config(
                 ]
         dependabot_config["updates"][-1].update({"cooldown": cooldown_config})
 
-    return yaml.dump(dependabot_config, stream)
-
 
 def build_dependabot_file(
     repo,

test_cooldown.py

@@ -3,6 +3,7 @@
 import unittest
 
 from dependabot_file import validate_cooldown_config
+from ruamel.yaml import YAML
 
 
 class TestValidateCooldownConfig(unittest.TestCase):
@@ -12,6 +13,12 @@ def test_valid_default_days_only(self):
         """Test valid cooldown with just default-days"""
         validate_cooldown_config({"default-days": 3})
 
+    def test_valid_ruamel_commented_map(self):
+        """Test that ruamel.yaml CommentedMap is accepted as a valid mapping"""
+        yaml = YAML()
+        cooldown = yaml.load(b"default-days: 5\nsemver-major-days: 14\n")
+        validate_cooldown_config(cooldown)
+
     def test_valid_all_days(self):
         """Test valid cooldown with all day parameters"""
         validate_cooldown_config(

test_dependabot_file.py

@@ -160,26 +160,20 @@ def test_build_dependabot_file_with_weird_space_indent_existing_config_bundler_w
     def test_build_dependabot_file_with_incorrect_indentation_in_extra_dependabot_config_file(
         self,
     ):
-        """Test incorrect indentation on extra_dependabot_config"""
-        repo = MagicMock()
-        repo.file_contents.side_effect = lambda f, filename="Gemfile": f == filename
-
-        # expected_result maintains existing ecosystem with custom configuration
-        # and adds new ecosystem
-        extra_dependabot_config = MagicMock()
-        extra_dependabot_config.content = base64.b64encode(b"""
+        """Test incorrect indentation on extra_dependabot_config is caught during YAML loading"""
+        # In production, extra_dependabot_config is loaded from a file via
+        # ruamel.yaml in evergreen.py before being passed to build_dependabot_file.
+        # Verify that malformed YAML raises an error at the loading stage.
+        yaml_loader = ruamel.yaml.YAML()
+        with self.assertRaises(ruamel.yaml.YAMLError):
+            yaml_loader.load(b"""
 npm:
 type: 'npm'
   url: 'https://yourprivateregistry/npm/'
   username: '${{secrets.username}}'
   password: '${{secrets.password}}'
   """)
 
-        with self.assertRaises(ruamel.yaml.YAMLError):
-            build_dependabot_file(
-                repo, False, [], {}, None, "weekly", "", [], extra_dependabot_config
-            )
-
     @patch.dict(os.environ, {"DEPENDABOT_CONFIG_FILE": "dependabot-config.yaml"})
     def test_build_dependabot_file_with_extra_dependabot_config_file(self):
         """Test that the dependabot.yaml file is built correctly with extra configurations from extra_dependabot_config"""