Fix Review Findings From the Develop-to-Main Promotion PR

repo-config/configure.sh

@@ -65,26 +65,47 @@ main_ruleset="$script_dir/main.json"
 settings_file="$script_dir/settings.json"
 
 # ----- Resolve the declared description (optional, shared by apply and check) -----
-# Per GOVERNANCE.md "Repository Details", once a repo declares registry/repos.json's `description` field, that field becomes the About panel's source rather than the README.
-# The audit's description_findings() (spec/audit.py) measures the README, About, and Docker Hub mirror set against that same field.
-# A repo with no declared field is left untouched here, so the README stays its source of truth.
+# Absence keeps the About panel following the README.
 description=""
 if [ -f "$registry" ]; then
-    # Trimmed defensively even though spec/validate.py already rejects an untrimmed value.
-    # A registry edited ahead of its next validate.py run still resolves to the same canonical value spec/audit.py compares against.
-    if ! description="$(jq -r --arg n "$name" \
-        '(.repos[] | select(.name==$n) | .description) // "" | gsub("^\\s+|\\s+$"; "")' "$registry")"; then
-        echo "Failed to read description from $registry (invalid JSON?)." >&2
+    # Fails loud on a duplicate name (already a validate.py DEFECT) rather than picking one entry over the other.
+    if ! match_count="$(jq -r --arg n "$name" '[.repos[] | select(.name==$n)] | length' "$registry")"; then
+        echo "Failed to read $registry (invalid JSON?)." >&2
         exit 1
     fi
-    # The trim above only strips leading/trailing whitespace, so an embedded newline or carriage return survives it.
-    # Caught here rather than left to reach `gh api` as a multi-line value.
-    case "$description" in
-        *$'\n'* | *$'\r'*)
-            echo "The declared description for $name in $registry carries an embedded newline. Fix it there (spec/validate.py rejects this once run)." >&2
+    if [ "$match_count" -gt 1 ]; then
+        echo "$match_count registry entries named $name in $registry. Resolve the duplicate before its description can be read (spec/validate.py rejects this once run)." >&2
+        exit 1
+    fi
+    if ! declared="$(jq -r --arg n "$name" '.repos[] | select(.name==$n) | has("description")' "$registry")"; then
+        echo "Failed to read $registry (invalid JSON?)." >&2
+        exit 1
+    fi
+    if [ "$declared" = "true" ]; then
+        # Exactly one match is already established above, so select() itself yields exactly one value here.
+        # No trim: this only ever validates the value against spec/validate.py's contract, never normalizes it.
+        # A non-string value (including an explicit null) resolves to empty here, caught by the same guard.
+        # -j plus the trailing sentinel keeps command substitution from stripping a genuine trailing newline.
+        if ! description="$(jq -j --arg n "$name" \
+            '(.repos[] | select(.name==$n) | .description) | if type == "string" then . else empty end' \
+            "$registry" && printf x)"; then
+            echo "Failed to read description from $registry (invalid JSON?)." >&2
             exit 1
-            ;;
-    esac
+        fi
+        description="${description%x}"
+        case "$description" in
+            "" | [[:space:]]* | *[[:space:]])
+                echo "The declared description for $name in $registry is not a non-empty string with no leading or trailing whitespace. Fix it there (spec/validate.py rejects this once run)." >&2
+                exit 1
+                ;;
+        esac
+        case "$description" in
+            *$'\n'* | *$'\r'*)
+                echo "The declared description for $name in $registry carries an embedded newline. Fix it there (spec/validate.py rejects this once run)." >&2
+                exit 1
+                ;;
+        esac
+    fi
 fi
 
 # ----- Ruleset id lookup (shared by apply and check) -----
@@ -181,8 +202,7 @@ cmd_apply() {
         payload="$(jq --argjson d "$disc" '. + {has_discussions: $d}' "$settings_file")"
         echo "Warning: $repo has no 'main' branch. Leaving default_branch unchanged." >&2
     fi
-    # The About description, only once a repo declares registry/repos.json's `description` (see the resolution above).
-    # Left untouched otherwise, so a repo that has not adopted the field yet keeps its hand-set (or README-derived) description.
+    # Applies only once a repo declares the field (see the resolution above).
     if [ -n "$description" ]; then
         payload="$(jq --arg desc "$description" '. + {description: $desc}' <<<"$payload")"
     fi
@@ -303,15 +323,13 @@ check_settings() {
     if gh api "repos/$repo/branches/main" --jq '.name' >/dev/null 2>&1; then
         assert "default_branch = main" test "$(jq -r '.default_branch' <<<"$live")" = main
     fi
-    # The About description, only where the registry declares one (see the resolution above).
-    # A repo that has not adopted the field is a manual-verify note, exactly as secrets are: nothing declared here to check against.
-    # The two reasons `$description` can be empty are told apart, since "no registry" and "no field for this repo" call for different follow-up.
+    # $description is empty for two different reasons, told apart below since each needs different follow-up.
     if [ -n "$description" ]; then
         assert "description = '$description'" test "$(jq -r '.description' <<<"$live")" = "$description"
     elif [ ! -f "$registry" ]; then
-        note "description: no $registry to read (pass a plain repo argument or run from a hub checkout) - verify manually"
+        note "description: no $registry to read (it resolves relative to this script, not from the repo argument). Run from a hub checkout for it to exist, and verify manually."
     else
-        note "description: no registry/repos.json description declared for $name - verify manually (falls back to the README tagline, see GOVERNANCE.md 'Repository Details')"
+        note "description: no matching registry entry or no declared description key for $name (falls back to the README tagline, see GOVERNANCE.md 'Repository Details'). Verify manually."
     fi
 }
 

scripts/tests/test_spec_validate.py

@@ -154,6 +154,28 @@ def test_whitespace_only_is_rejected(self) -> None:
             ["Fixture: description must be a non-empty string"],
         )
 
+    def test_an_explicit_null_is_rejected_rather_than_read_as_absent(self) -> None:
+        self.assertEqual(
+            validate.description_errors("Fixture", None),
+            ["Fixture: description must be a non-empty string"],
+        )
+
+    def test_for_repo_an_absent_key_produces_no_errors(self) -> None:
+        self.assertEqual(validate.description_errors_for_repo({}, "Fixture"), [])
+
+    def test_for_repo_an_explicit_null_is_rejected_rather_than_read_as_absent(self) -> None:
+        # Locks in the presence-vs-None guard: this regresses to `[]` if it is ever weakened back to `is not None`.
+        self.assertEqual(
+            validate.description_errors_for_repo({"description": None}, "Fixture"),
+            ["Fixture: description must be a non-empty string"],
+        )
+
+    def test_a_non_string_is_rejected(self) -> None:
+        self.assertEqual(
+            validate.description_errors("Fixture", 42),
+            ["Fixture: description must be a non-empty string"],
+        )
+
     def test_an_inline_markdown_link_is_rejected(self) -> None:
         self.assertEqual(
             validate.description_errors("Fixture", "See [docs](https://example.test) for more."),

spec/audit.py

@@ -41,6 +41,8 @@
 from datetime import UTC, datetime
 from typing import Any
 
+import validate  # sibling, import-safe (its main is guarded)
+
 ROOT = pathlib.Path(__file__).resolve().parent.parent
 
 SETTINGS_KEYS = [
@@ -1303,7 +1305,21 @@ def description_findings(doc_texts, entry, live, slug):
     declared value is canonical on its own and does not need the README to establish it.
     """
     findings = []
-    declared = (entry.get("description") or "").strip() or None
+    declared = None
+    if "description" in entry:
+        # Delegates to validate.py's own contract instead of re-checking a second, easily-incomplete copy of it
+        # (an earlier version here missed the link and length rules, accepting either as canonical).
+        shape_errors = validate.description_errors("registry", entry["description"])
+        if shape_errors:
+            findings += [
+                (
+                    "DEFECT",
+                    f"{msg}. Treated as undeclared here, since it fails spec/validate.py's contract.",
+                )
+                for msg in shape_errors
+            ]
+        else:
+            declared = entry["description"]
     readme_want = None
     if "README.md" in doc_texts:
         title, intro = title_and_intro(doc_texts["README.md"])
@@ -4214,6 +4230,55 @@ def _selftest():
             {"description": "Anything."},
             0,
         ),
+        (
+            "a non-string declared field is reported rather than crashing",
+            desc_readme,
+            {"description": 42},
+            {"description": "A short tagline."},
+            1,
+        ),
+        (
+            "an explicit null declared field is reported rather than read as absent",
+            desc_readme,
+            {"description": None},
+            {"description": "A short tagline."},
+            1,
+        ),
+        (
+            "a whitespace-only declared field is reported rather than read as absent",
+            desc_readme,
+            {"description": "   "},
+            {"description": "A short tagline."},
+            1,
+        ),
+        (
+            "a padded declared field is a DEFECT rather than silently trimmed",
+            desc_readme,
+            {"description": "  A short tagline.  "},
+            {"description": "A short tagline."},
+            1,
+        ),
+        (
+            "a declared field with an embedded newline is a DEFECT rather than silently accepted",
+            desc_readme,
+            {"description": "A short tagline.\nA second line."},
+            {"description": "A short tagline."},
+            1,
+        ),
+        (
+            "a declared field carrying a Markdown link is a DEFECT rather than silently canonical",
+            desc_readme,
+            {"description": "See [docs](https://example.test) for more."},
+            {"description": "A short tagline."},
+            1,
+        ),
+        (
+            "a declared field over the 100-char cap is a DEFECT rather than silently canonical",
+            desc_readme,
+            {"description": "a" * 101},
+            {"description": "A short tagline."},
+            1,
+        ),
     ]
     for label, doc_texts_fx, entry_fx, live_fx, wantn in desc_cases:
         got = description_findings(doc_texts_fx, entry_fx, live_fx, "owner/Fixture")
@@ -4225,6 +4290,17 @@ def _selftest():
         if len(got) != wantn:
             for _, t in got:
                 print(f"         {t}")
+    # A null declared field is a DEFECT via validate.py's own contract, not a silent LETTER-only "About mismatch".
+    null_declared = description_findings(
+        desc_readme, {"description": None}, {"description": "A short tagline."}, "owner/Fixture"
+    )
+    if not any(
+        k == "DEFECT" and "description must be a non-empty string" in t for k, t in null_declared
+    ):
+        ok = False
+        print(f"  FAIL description: null-declared-field DEFECT contract -> {null_declared}")
+    else:
+        print("  ok   description: a null declared field is a DEFECT via validate.py's contract")
     # The declared field, once present, is what the wording names as the source - not "the README".
     declared_mismatch = description_findings(
         desc_readme,

spec/validate.py

@@ -56,6 +56,17 @@ def is_str_list(v):
     return isinstance(v, list) and all(isinstance(x, str) for x in v)
 
 
+def description_errors_for_repo(repo, name):
+    """The per-repo optional-field guard: an explicit `"description": null` is declared-but-invalid, not absent.
+
+    Presence (`"description" in repo`) is the test, not `repo.get("description") is not None`, so a `null` reaches
+    description_errors() rather than being read as though the field were never declared.
+    """
+    if "description" not in repo:
+        return []
+    return description_errors(name, repo["description"])
+
+
 def description_errors(name, desc):
     """Shape errors for a registry entry's optional `description` (GOVERNANCE.md "Repository Details").
 
@@ -461,16 +472,21 @@ def check_secret_set(label, entry, need_kind):
         )
 
     seen_identities = set()
+    seen_names = set()
     for i, repo in enumerate(repos["repos"]):
         if not isinstance(repo, dict):
             errors.append(f"repo #{i} is not an object")
             continue
         name = repo.get("name", f"#{i}")
         # This name only labels every error message below.
         # The membership check (spec/audit.py's membership_findings()) keys by owner/repo instead, parsed from url the same way this loop does.
+        # A duplicate is still an error, though, since repo-config/configure.sh and spec/audit.py's own per-repo entry lookup both key off it.
         if not isinstance(repo.get("name"), str) or not repo["name"].strip():
             errors.append(f"repo #{i}: missing or empty 'name'")
             continue
+        if name in seen_names:
+            errors.append(f"{name}: duplicate registry entry for name '{name}'")
+        seen_names.add(name)
         if not isinstance(repo.get("url"), str) or not repo["url"].strip():
             errors.append(f"{name}: missing or empty 'url'")
             continue
@@ -503,9 +519,7 @@ def check_secret_set(label, entry, need_kind):
         if effective_model == "operational" and eol is None:
             errors.append(f"{name}: operational repo must declare lineEndings (lf or crlf)")
         # Optional per GOVERNANCE.md "Repository Details": a repo that has not adopted the field yet is unaffected, since spec/audit.py's description_findings() falls back to the README tagline for it.
-        desc = repo.get("description")
-        if desc is not None:
-            errors.extend(description_errors(name, desc))
+        errors.extend(description_errors_for_repo(repo, name))
 
         status = repo.get("status")
         if status is None: