Skip to content

Commit fbd6cd9

Browse files
Jakub Baranowskiclaude
andcommitted
fix(ci): make the extension version guard usable as a required check and quoting-proof
Address Copilot review round 1 on #4395. Workflow: drop the `paths: extensions/**` filter. A required status check that is skipped by path filtering stays in "Expected" state and blocks every PR that does not touch extensions/**, which defeats the point of making the guard a merge requirement. The job now runs on every pull request; the script already reports success when nothing under extensions/ changed, so unrelated PRs pass in one short job. Script: read the changed-path list with `git diff --name-only -z`. With git's default core.quotePath, a path containing non-ASCII or control characters is C-quoted with the quotes included (`"extensions/demo/caf\303\251.txt"`), so its first component was no longer `extensions` and an unbumped change to such a file escaped the guard. NUL-delimited output is emitted verbatim; paths are decoded with surrogateescape so an undecodable byte cannot crash the check, and only the ASCII `extensions/<id>/` prefix is ever interpreted. Tests: pin both behaviors. The non-ASCII case fails against the previous script and passes now; the no-extension-changes case backs the workflow change. core.quotePath is pinned to true in the fixture repo so the regression exercises the quoting path even where a developer's global config disables it. Refs #4345 Assisted-by: Claude Code (model: claude-fable-5-1) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 4383296 commit fbd6cd9

3 files changed

Lines changed: 61 additions & 11 deletions

File tree

‎.github/scripts/check_extension_version_bump.py‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
import json
3838
import subprocess
3939
import sys
40-
from pathlib import Path
40+
from pathlib import PurePosixPath
4141

4242
import yaml
4343
from packaging.version import InvalidVersion, Version
@@ -46,10 +46,29 @@
4646
CATALOG_PATH = f"{EXTENSIONS_ROOT}/catalog.json"
4747

4848

49-
def _git(*args: str) -> str:
50-
return subprocess.run(
51-
["git", *args], check=True, capture_output=True, text=True
49+
def _changed_paths(base_ref: str, head_ref: str) -> list[str]:
50+
"""Paths under extensions/ that differ between *base_ref* and *head_ref*.
51+
52+
Uses NUL-delimited output (``-z``): without it git C-quotes any path
53+
containing non-ASCII or control characters (``"extensions/x/caf\\303\\251"``,
54+
quotes included), so the leading component would no longer equal
55+
``extensions`` and that change would silently escape the guard. Paths
56+
are decoded with surrogateescape so an undecodable byte can never crash
57+
the check; only the ASCII ``extensions/<id>/`` prefix is interpreted.
58+
"""
59+
raw = subprocess.run(
60+
[
61+
"git", "diff", "--name-only", "-z", "--no-renames",
62+
base_ref, head_ref, "--", EXTENSIONS_ROOT,
63+
],
64+
check=True,
65+
capture_output=True,
5266
).stdout
67+
return [
68+
chunk.decode("utf-8", errors="surrogateescape")
69+
for chunk in raw.split(b"\0")
70+
if chunk
71+
]
5372

5473

5574
def _show(ref: str, path: str) -> str | None:
@@ -87,13 +106,10 @@ def main(argv: list[str]) -> int:
87106
errors: list[str] = []
88107

89108
# -- Invariant 1: content change requires a version bump ---------------
90-
changed = _git(
91-
"diff", "--name-only", "--no-renames", base_ref, head_ref, "--", EXTENSIONS_ROOT
92-
).splitlines()
93109
changed_ids = {
94110
parts[1]
95-
for line in changed
96-
if len(parts := Path(line.strip()).parts) >= 3 and parts[0] == EXTENSIONS_ROOT
111+
for path in _changed_paths(base_ref, head_ref)
112+
if len(parts := PurePosixPath(path).parts) >= 3 and parts[0] == EXTENSIONS_ROOT
97113
}
98114

99115
for ext_id in sorted(changed_ids):

‎.github/workflows/extension-version-guard.yml‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,13 @@ permissions:
99
# Content changes shipped without a bump go silently stale on every
1010
# project that already installed the extension (#4345). This guard turns
1111
# "please remember to bump" into a merge requirement.
12+
#
13+
# Deliberately no `paths:` filter: a required status check that is skipped
14+
# by path filtering stays in "Expected" state and blocks every PR that does
15+
# not touch extensions/**. The check runs on every pull request instead and
16+
# the script reports success when nothing under extensions/ changed.
1217
on:
1318
pull_request:
14-
paths:
15-
- "extensions/**"
1619

1720
jobs:
1821
version-bump:

‎tests/contract/test_extension_version_guard_script.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ def guard_repo(tmp_path: Path) -> tuple[Path, str]:
7575
_git(repo, "config", "user.email", "guard-tests@example.com")
7676
_git(repo, "config", "user.name", "Guard Tests")
7777
_git(repo, "config", "commit.gpgsign", "false")
78+
# git's default; pinned so the non-ASCII regression below exercises the
79+
# C-quoting code path even on machines whose global config disables it.
80+
_git(repo, "config", "core.quotePath", "true")
7881

7982
_write_extension(repo, "demo", "1.0.0", "echo base")
8083
_write_extension(repo, "scratch", "1.0.0", "echo base") # not in catalog
@@ -114,6 +117,34 @@ def test_unbumped_content_change_fails(guard_repo):
114117
assert "extensions/demo/extension.yml" in result.stdout
115118

116119

120+
def test_unbumped_non_ascii_filename_fails(guard_repo):
121+
"""With core.quotePath (git's default) `git diff --name-only` C-quotes a
122+
path like extensions/demo/café.txt, quotes included, so a line-based
123+
parser no longer sees `extensions` as the first component and the
124+
change escapes the guard. The NUL-delimited diff must still catch it."""
125+
repo, base = guard_repo
126+
(repo / "extensions" / "demo" / "café.txt").write_text("new\n", encoding="utf-8")
127+
_commit_all(repo, "add non-ascii file without bump")
128+
129+
result = _run_guard(repo, base)
130+
assert result.returncode == 1, result.stdout + result.stderr
131+
assert "did not increase" in result.stdout
132+
assert "extensions/demo/extension.yml" in result.stdout
133+
134+
135+
def test_no_extension_changes_passes(guard_repo):
136+
"""The workflow runs on every pull request (a path-filtered required check
137+
would block PRs that skip it), so a PR touching nothing under extensions/
138+
must pass rather than be reported as a violation."""
139+
repo, base = guard_repo
140+
(repo / "README.md").write_text("docs only\n", encoding="utf-8")
141+
_commit_all(repo, "unrelated change")
142+
143+
result = _run_guard(repo, base)
144+
assert result.returncode == 0, result.stdout + result.stderr
145+
assert "all invariants hold" in result.stdout
146+
147+
117148
def test_downgrade_fails(guard_repo):
118149
repo, base = guard_repo
119150
_write_extension(repo, "demo", "0.9.0", "echo changed")

0 commit comments

Comments
 (0)