Skip to content

Commit 1c10491

Browse files
committed
fix: address Copilot review round 8 findings
- Raise tag limit from 5 to 10 to match existing catalog entries; update publishing guides accordingly - Deduplicate tags in parse_tags() so duplicate submissions produce stable catalog output - Make _count_list_items() tolerant of non-bullet formats: count all non-empty lines when no bullets are present - Add 29 unit tests for catalog-validate.py covering parse_issue_body, tags, description, speckit_version, _count_list_items, and SSRF guard
1 parent 8278474 commit 1c10491

4 files changed

Lines changed: 198 additions & 12 deletions

File tree

.github/scripts/catalog-validate.py

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -376,8 +376,8 @@ def validate_tags(value: str) -> tuple[bool, str]:
376376
raw_tags = [t.strip().lower() for t in value.split(",") if t.strip()]
377377
if len(raw_tags) < 2:
378378
return False, "Please provide at least 2 tags."
379-
if len(raw_tags) > 5:
380-
return False, f"Too many tags ({len(raw_tags)}). Please provide 2-5 tags."
379+
if len(raw_tags) > 10:
380+
return False, f"Too many tags ({len(raw_tags)}). Please provide 2-10 tags."
381381
bad = [t for t in raw_tags if not re.match(r"^[a-z0-9-]+$", t)]
382382
if bad:
383383
return False, (
@@ -413,13 +413,16 @@ def validate_checklist(value: str, field_name: str) -> tuple[bool, str]:
413413

414414

415415
def _count_list_items(value: str) -> int:
416-
"""Count markdown list items (``- item``) in a textarea value."""
416+
"""Count items in a textarea value (bullets or non-empty lines)."""
417417
if not _present(value):
418418
return 0
419-
return sum(
420-
1 for line in value.splitlines()
421-
if line.strip().startswith("- ") or line.strip().startswith("* ")
422-
)
419+
lines = [line.strip() for line in value.splitlines() if line.strip()]
420+
# If any lines use bullet format, count only those
421+
bullets = [l for l in lines if l.startswith(("- ", "* "))]
422+
if bullets:
423+
return len(bullets)
424+
# Otherwise count all non-empty lines
425+
return len(lines)
423426

424427

425428
# ---------------------------------------------------------------------------
@@ -568,12 +571,12 @@ def _add(field: str, ok: bool, msg: str, *, severity: str = "error") -> None:
568571
# ---------------------------------------------------------------------------
569572

570573
def parse_tags(value: str) -> list[str]:
571-
"""Parse comma-separated tags into a sorted list."""
572-
return sorted(
574+
"""Parse comma-separated tags into a sorted, deduplicated list."""
575+
return sorted(set(
573576
t.strip().lower()
574577
for t in value.split(",")
575578
if t.strip()
576-
)
579+
))
577580

578581

579582
def _clean(value: str | None) -> str:

extensions/EXTENSION-PUBLISHING-GUIDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ Submit your extension by opening an issue using the **Extension Submission** tem
4949
- Description length (under 200 characters)
5050
- Required URLs are present and reachable
5151
- Spec Kit version constraint is valid
52-
- Tags format and count (2-5 lowercase tags)
52+
- Tags format and count (2-10 lowercase tags)
5353
- All required fields and checklists are completed
5454
2. If any checks fail, the issue gets a `needs-changes` label with details on what to fix. Edit the issue to correct the fields and validation re-runs automatically.
5555
3. Once all checks pass, the issue gets a `validated` label and a **pull request is created automatically** that:

presets/PUBLISHING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ Submit your preset by opening an issue using the **Preset Submission** template:
5353
- Description length (under 200 characters)
5454
- Required URLs are present and reachable
5555
- Spec Kit version constraint is valid
56-
- Tags format and count (2-5 lowercase tags)
56+
- Tags format and count (2-10 lowercase tags)
5757
- All required fields and checklists are completed
5858
2. If any checks fail, the issue gets a `needs-changes` label with details on what to fix. Edit the issue to correct the fields and validation re-runs automatically.
5959
3. Once all checks pass, the issue gets a `validated` label and a **pull request is created automatically** that:

tests/test_catalog_validate.py

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
"""Tests for .github/scripts/catalog-validate.py."""
2+
3+
from __future__ import annotations
4+
5+
import importlib
6+
import ipaddress
7+
import sys
8+
import types
9+
from pathlib import Path
10+
from unittest.mock import patch
11+
12+
import pytest
13+
14+
# ---------------------------------------------------------------------------
15+
# Import the script as a module
16+
# ---------------------------------------------------------------------------
17+
18+
SCRIPT = Path(__file__).resolve().parents[1] / ".github" / "scripts" / "catalog-validate.py"
19+
20+
21+
@pytest.fixture(scope="module")
22+
def cv():
23+
"""Import catalog-validate.py as a module."""
24+
spec = importlib.util.spec_from_file_location("catalog_validate", SCRIPT)
25+
mod = importlib.util.module_from_spec(spec)
26+
spec.loader.exec_module(mod)
27+
return mod
28+
29+
30+
# ---------------------------------------------------------------------------
31+
# parse_issue_body
32+
# ---------------------------------------------------------------------------
33+
34+
35+
class TestParseIssueBody:
36+
def test_basic_fields(self, cv):
37+
body = "### Name\nAlice\n### Version\n1.0.0\n"
38+
result = cv.parse_issue_body(body)
39+
assert result["Name"] == "Alice"
40+
assert result["Version"] == "1.0.0"
41+
42+
def test_multiline_field(self, cv):
43+
body = "### Description\nLine one\nLine two\n### End\nval"
44+
result = cv.parse_issue_body(body)
45+
assert "Line one\nLine two" == result["Description"]
46+
47+
def test_empty_body(self, cv):
48+
assert cv.parse_issue_body("") == {}
49+
50+
def test_checkbox_field(self, cv):
51+
body = "### Checklist\n- [X] Item 1\n- [ ] Item 2\n"
52+
result = cv.parse_issue_body(body)
53+
assert "- [X] Item 1" in result["Checklist"]
54+
55+
56+
# ---------------------------------------------------------------------------
57+
# parse_tags / validate_tags
58+
# ---------------------------------------------------------------------------
59+
60+
61+
class TestTags:
62+
def test_parse_tags_dedup(self, cv):
63+
assert cv.parse_tags("foo, foo, bar") == ["bar", "foo"]
64+
65+
def test_parse_tags_sorted(self, cv):
66+
assert cv.parse_tags("z, a, m") == ["a", "m", "z"]
67+
68+
def test_validate_tags_too_few(self, cv):
69+
ok, _ = cv.validate_tags("single")
70+
assert not ok
71+
72+
def test_validate_tags_too_many(self, cv):
73+
ok, _ = cv.validate_tags(", ".join(f"tag{i}" for i in range(11)))
74+
assert not ok
75+
76+
def test_validate_tags_max_10(self, cv):
77+
ok, _ = cv.validate_tags(", ".join(f"tag{i}" for i in range(10)))
78+
assert ok
79+
80+
def test_validate_tags_bad_chars(self, cv):
81+
ok, _ = cv.validate_tags("good, BAD CHARS!")
82+
assert not ok
83+
84+
85+
# ---------------------------------------------------------------------------
86+
# validate_description
87+
# ---------------------------------------------------------------------------
88+
89+
90+
class TestValidateDescription:
91+
def test_empty(self, cv):
92+
ok, _ = cv.validate_description("")
93+
assert not ok
94+
95+
def test_valid(self, cv):
96+
ok, _ = cv.validate_description("Short description")
97+
assert ok
98+
99+
def test_over_limit_new(self, cv):
100+
ok, _ = cv.validate_description("x" * 201)
101+
assert not ok
102+
103+
def test_over_limit_update_warns(self, cv):
104+
ok, msg = cv.validate_description("x" * 201, is_update=True)
105+
assert ok
106+
assert "Consider shortening" in msg
107+
108+
109+
# ---------------------------------------------------------------------------
110+
# validate_speckit_version
111+
# ---------------------------------------------------------------------------
112+
113+
114+
class TestValidateSpeckitVersion:
115+
def test_valid(self, cv):
116+
ok, _ = cv.validate_speckit_version(">=0.6.0")
117+
assert ok
118+
119+
def test_valid_multi(self, cv):
120+
ok, _ = cv.validate_speckit_version(">=0.6.0,<1.0.0")
121+
assert ok
122+
123+
def test_invalid(self, cv):
124+
ok, _ = cv.validate_speckit_version("not-a-version")
125+
assert not ok
126+
127+
def test_empty(self, cv):
128+
ok, _ = cv.validate_speckit_version("")
129+
assert not ok
130+
131+
132+
# ---------------------------------------------------------------------------
133+
# _count_list_items
134+
# ---------------------------------------------------------------------------
135+
136+
137+
class TestCountListItems:
138+
def test_bullets(self, cv):
139+
assert cv._count_list_items("- one\n- two\n- three") == 3
140+
141+
def test_asterisks(self, cv):
142+
assert cv._count_list_items("* one\n* two") == 2
143+
144+
def test_plain_lines(self, cv):
145+
assert cv._count_list_items("one\ntwo\nthree") == 3
146+
147+
def test_empty(self, cv):
148+
assert cv._count_list_items("") == 0
149+
150+
def test_mixed_blank(self, cv):
151+
assert cv._count_list_items("- one\n\n- two") == 2
152+
153+
154+
# ---------------------------------------------------------------------------
155+
# SSRF guard (_is_safe_redirect_target)
156+
# ---------------------------------------------------------------------------
157+
158+
159+
class TestSSRFGuard:
160+
def test_rejects_private_ip(self, cv):
161+
assert not cv._is_safe_redirect_target("http://127.0.0.1/evil")
162+
163+
def test_rejects_non_http(self, cv):
164+
assert not cv._is_safe_redirect_target("ftp://example.com/file")
165+
166+
def test_rejects_no_hostname(self, cv):
167+
assert not cv._is_safe_redirect_target("http:///path")
168+
169+
def test_allows_public(self, cv):
170+
# Mock DNS to return a public IP
171+
fake_addr = [(None, None, None, None, ("93.184.216.34", 0))]
172+
with patch.object(cv.socket, "getaddrinfo", return_value=fake_addr):
173+
assert cv._is_safe_redirect_target("https://example.com")
174+
175+
def test_rejects_multicast(self, cv):
176+
fake_addr = [(None, None, None, None, ("224.0.0.1", 0))]
177+
with patch.object(cv.socket, "getaddrinfo", return_value=fake_addr):
178+
assert not cv._is_safe_redirect_target("https://multicast.test")
179+
180+
def test_rejects_unspecified(self, cv):
181+
fake_addr = [(None, None, None, None, ("0.0.0.0", 0))]
182+
with patch.object(cv.socket, "getaddrinfo", return_value=fake_addr):
183+
assert not cv._is_safe_redirect_target("https://zero.test")

0 commit comments

Comments
 (0)