fix: address Copilot review round 9 findings

- Deduplicate tags in validate_tags() before counting; surface a
  message when duplicates are removed
- Require at least one checkbox item in validate_checklist() so
  missing/mangled checkbox syntax fails instead of silently passing
- Use packaging.version.Version for semver comparison with fallback,
  fixing incorrect pre-release handling (e.g. 1.0.0-alpha vs 1.0.0)
- Omit version key from tools when no version is supplied instead of
  writing a synthetic >=0.0.0 constraint
- Fail closed in _is_safe_redirect_target() on DNS resolution failure
  to prevent DNS rebinding bypass
- Re-add 'validated' label on issue edits (remove + add) so
  catalog-pr.yml is retriggered to update the generated PR
- Add tests for tag dedup validation and DNS-fail-closed behavior

Author: mnriem

.github/scripts/catalog-validate.py

@@ -157,7 +157,12 @@ def _present(value: str | None) -> bool:
 
 def _parse_semver(version: str) -> tuple[int, ...]:
     """Parse a semver string into a comparable tuple of ints."""
-    # Strip pre-release/build metadata for comparison
+    try:
+        from packaging.version import Version
+        return Version(version)
+    except (ImportError, Exception):
+        pass
+    # Fallback: strip pre-release/build metadata for comparison
     base = version.split("-")[0].split("+")[0]
     return tuple(int(p) for p in base.split("."))
 
@@ -256,7 +261,7 @@ def _is_safe_redirect_target(url: str) -> bool:
             if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved or ip.is_unspecified or ip.is_multicast:
                 return False
     except (socket.gaierror, ValueError):
-        pass
+        return False  # fail closed: unresolvable targets are blocked
     return True
 
 
@@ -373,17 +378,23 @@ def validate_hooks_count(value: str) -> tuple[bool, str]:
 def validate_tags(value: str) -> tuple[bool, str]:
     if not _present(value):
         return False, "Tags are required."
-    raw_tags = [t.strip().lower() for t in value.split(",") if t.strip()]
+    raw_tags = list(dict.fromkeys(
+        t.strip().lower() for t in value.split(",") if t.strip()
+    ))  # dedupe preserving order
+    dupes_removed = len([t.strip().lower() for t in value.split(",") if t.strip()]) - len(raw_tags)
     if len(raw_tags) < 2:
-        return False, "Please provide at least 2 tags."
+        return False, "Please provide at least 2 unique tags."
     if len(raw_tags) > 10:
-        return False, f"Too many tags ({len(raw_tags)}). Please provide 2-10 tags."
+        return False, f"Too many tags ({len(raw_tags)} unique). Please provide 2-10 tags."
     bad = [t for t in raw_tags if not re.match(r"^[a-z0-9-]+$", t)]
     if bad:
         return False, (
             f"Tags must be lowercase alphanumeric with hyphens: {', '.join(bad)}"
         )
-    return True, f"Tags: {', '.join(raw_tags)}."
+    msg = f"Tags: {', '.join(raw_tags)}."
+    if dupes_removed:
+        msg += f" ({dupes_removed} duplicate(s) removed.)"
+    return True, msg
 
 
 def validate_license(value: str) -> tuple[bool, str]:
@@ -403,6 +414,8 @@ def validate_checklist(value: str, field_name: str) -> tuple[bool, str]:
     if not _present(value):
         return False, f"{field_name} is required."
     lines = [l.strip() for l in value.splitlines() if l.strip().startswith("- [")]
+    if not lines:
+        return False, f"{field_name} must contain at least one checkbox item."
     unchecked = [l for l in lines if l.startswith("- [ ]")]
     if unchecked:
         items = "\n".join(f"  - {l[5:].strip()}" for l in unchecked)
@@ -642,14 +655,16 @@ def _build_extension_entry(
             m = _tool_re.match(line)
             if m:
                 name = m.group("name").strip()
-                version = m.group("version") or ">=0.0.0"
-                version = version.strip()
+                version = m.group("version")
+                version = version.strip() if version else None
                 req_str = (m.group("req") or "required").lower()
-                tools_list.append({
+                tool_entry: dict = {
                     "name": name,
-                    "version": version,
                     "required": req_str != "optional",
-                })
+                }
+                if version:
+                    tool_entry["version"] = version
+                tools_list.append(tool_entry)
             else:
                 # Fallback: comma-separated "name>=version"
                 for part in line.split(","):
@@ -668,7 +683,6 @@ def _build_extension_entry(
                     else:
                         tools_list.append({
                             "name": part,
-                            "version": ">=0.0.0",
                             "required": True,
                         })
         if tools_list:

.github/workflows/catalog-validate.yml

@@ -93,14 +93,21 @@ jobs:
                   name: 'needs-changes',
                 });
               }
-              if (!currentLabels.includes('validated')) {
-                await github.rest.issues.addLabels({
+              if (currentLabels.includes('validated')) {
+                // Remove + re-add to retrigger catalog-pr on edits
+                await github.rest.issues.removeLabel({
                   owner: context.repo.owner,
                   repo: context.repo.repo,
                   issue_number: context.issue.number,
-                  labels: ['validated'],
+                  name: 'validated',
                 });
               }
+              await github.rest.issues.addLabels({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                issue_number: context.issue.number,
+                labels: ['validated'],
+              });
             } else {
               if (currentLabels.includes('validated')) {
                 await github.rest.issues.removeLabel({
@@ -201,14 +208,21 @@ jobs:
                   name: 'needs-changes',
                 });
               }
-              if (!currentLabels.includes('validated')) {
-                await github.rest.issues.addLabels({
+              if (currentLabels.includes('validated')) {
+                // Remove + re-add to retrigger catalog-pr on edits
+                await github.rest.issues.removeLabel({
                   owner: context.repo.owner,
                   repo: context.repo.repo,
                   issue_number: context.issue.number,
-                  labels: ['validated'],
+                  name: 'validated',
                 });
               }
+              await github.rest.issues.addLabels({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                issue_number: context.issue.number,
+                labels: ['validated'],
+              });
             } else {
               if (currentLabels.includes('validated')) {
                 await github.rest.issues.removeLabel({

tests/test_catalog_validate.py

@@ -81,6 +81,17 @@ def test_validate_tags_bad_chars(self, cv):
         ok, _ = cv.validate_tags("good, BAD CHARS!")
         assert not ok
 
+    def test_validate_tags_dedup_count(self, cv):
+        # 3 raw tags but only 2 unique — should still pass (>= 2)
+        ok, msg = cv.validate_tags("foo, foo, bar")
+        assert ok
+        assert "duplicate" in msg.lower()
+
+    def test_validate_tags_dedup_too_few(self, cv):
+        # All dupes of same tag — only 1 unique
+        ok, _ = cv.validate_tags("foo, foo, foo")
+        assert not ok
+
 
 # ---------------------------------------------------------------------------
 # validate_description
@@ -181,3 +192,9 @@ def test_rejects_unspecified(self, cv):
         fake_addr = [(None, None, None, None, ("0.0.0.0", 0))]
         with patch.object(cv.socket, "getaddrinfo", return_value=fake_addr):
             assert not cv._is_safe_redirect_target("https://zero.test")
+
+    def test_rejects_unresolvable(self, cv):
+        """DNS failure should fail closed (block, not allow)."""
+        import socket as _socket
+        with patch.object(cv.socket, "getaddrinfo", side_effect=_socket.gaierror):
+            assert not cv._is_safe_redirect_target("https://unresolvable.test")