diff --git a/repo-config/configure.sh b/repo-config/configure.sh index e5de18eb..4995ad59 100755 --- a/repo-config/configure.sh +++ b/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 } diff --git a/scripts/tests/test_spec_validate.py b/scripts/tests/test_spec_validate.py index c7a29340..44da6a01 100755 --- a/scripts/tests/test_spec_validate.py +++ b/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."), diff --git a/spec/audit.py b/spec/audit.py index e5f2bc47..ee0b8650 100755 --- a/spec/audit.py +++ b/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, diff --git a/spec/validate.py b/spec/validate.py index 70648088..d686dd5b 100755 --- a/spec/validate.py +++ b/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,6 +472,7 @@ 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") @@ -468,9 +480,13 @@ def check_secret_set(label, entry, need_kind): 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: