diff --git a/.cspell-repo-terms.txt b/.cspell-repo-terms.txt index f3be486fa..ec2923035 100644 --- a/.cspell-repo-terms.txt +++ b/.cspell-repo-terms.txt @@ -1009,6 +1009,12 @@ hostnames dataprotection # dorny/paths-filter — GitHub Actions dependency used in policy-validation.yml dorny +# git fetch destination-ref syntax, spell-check.yml base fetch +refspec +# Step id in ci.yml's markdown-link-check merge-base scoping (#3620) +linkbase +# Test name in the TypeScript approval-protocol suite (#3200) +unpermitted writerow wslc x86 diff --git a/.github/workflows/spell-check.yml b/.github/workflows/spell-check.yml index 5b9b12721..c713e802e 100644 --- a/.github/workflows/spell-check.yml +++ b/.github/workflows/spell-check.yml @@ -42,10 +42,22 @@ jobs: # `git merge-base` fails and the diff falls back to the base tip -- which is # what made this job spell-check the whole repository on any branch a few # commits behind main. + # The refspec is explicit as hardening, not as a fix for an observed + # failure. `git fetch origin ` is only guaranteed to write + # FETCH_HEAD; whether it also updates `refs/remotes/origin/` + # depends on the configured remote.origin.fetch. On this workflow today + # that wildcard is intact, so the tracking ref does get updated and the + # plain form works. Naming the destination ref makes the next step's + # `--base origin/` correct regardless of how the remote is + # configured, which matters because a stale tracking ref would not fail + # loudly: `git merge-base` against one still succeeds, so the fallback + # warning below would not fire and the over-scan would look like a + # correctly scoped run. - name: Fetch PR base run: | set -e - git fetch origin "${{ github.base_ref }}" + git fetch --no-tags origin \ + "+${{ github.base_ref }}:refs/remotes/origin/${{ github.base_ref }}" - name: Compute changed lines id: diff diff --git a/scripts/ci/changed_lines.py b/scripts/ci/changed_lines.py index dc6e23562..47ec7d05c 100644 --- a/scripts/ci/changed_lines.py +++ b/scripts/ci/changed_lines.py @@ -5,6 +5,7 @@ from __future__ import annotations import argparse +import os import subprocess import sys from pathlib import Path @@ -31,10 +32,26 @@ def pathspecs_for_extensions(extensions: Iterable[str]) -> list[str]: def extract_added_lines(diff_text: str) -> str: - """Extract only added content lines from a unified diff.""" + """Extract only added content lines from a unified diff. + + The `+++ b/path` file header is skipped by position rather than by prefix. + A prefix test cannot tell it apart from a genuinely added line whose own + content starts with `++`, which arrives as `+++...` and would be dropped: + the content would then never be checked, and a check that silently skips + input fails in the direction of missing what it exists to find. File + headers only appear before the first `@@` hunk of each file, so tracking + whether a hunk is open distinguishes them exactly. + """ added_lines: list[str] = [] + in_hunk = False for line in diff_text.splitlines(): - if line.startswith("+++"): + if line.startswith("@@"): + in_hunk = True + continue + if line.startswith("diff --git "): + in_hunk = False + continue + if not in_hunk and line.startswith("+++"): continue if line.startswith("+"): added_lines.append(line[1:]) @@ -75,10 +92,15 @@ def resolve_merge_base(repo: Path, base: str) -> str: # stderr, not stdout: without `--output` this script emits its result on # stdout, so a warning printed there is read back as one of the changed # file names (or as an added line) by whatever consumes it. - print( - f"warning: cannot resolve merge base with {base} ({message}); diffing against its tip instead", - file=sys.stderr, - ) + text = f"cannot resolve merge base with {base} ({message}); diffing against its tip instead" + print(f"warning: {text}", file=sys.stderr) + # Also surface it as a workflow annotation. On stderr alone this notice + # is buried in the step log, so an over-reporting run is indistinguishable + # from a correctly scoped one at the point where someone reads the check + # result -- which is why the scoping regression this guards against went + # unnoticed while it failed unrelated PRs. + if os.environ.get("GITHUB_ACTIONS") == "true": + print(f"::warning::changed_lines: {text}", file=sys.stderr) return base return result.stdout.strip() or base diff --git a/tests/ci/test_changed_lines.py b/tests/ci/test_changed_lines.py index 12be181c2..7d5f35fb2 100644 --- a/tests/ci/test_changed_lines.py +++ b/tests/ci/test_changed_lines.py @@ -52,6 +52,84 @@ def test_extract_added_lines_combines_multiple_files_without_diff_metadata() -> assert changed_lines.extract_added_lines(diff_text) == "New README tokenn.\nprint(\"neew token\")\n" +def test_added_line_whose_content_starts_with_plus_plus_is_not_dropped() -> None: + """A content line beginning with `++` arrives as `+++...`, like a file header. + + Skipping every `+++` by prefix silently discards that content, so the words + on it are never spell-checked. That is the direction this script must not + fail in: over-reporting is noisy, under-reporting means the check passes on + text nobody looked at. File headers only appear before the first `@@`, so + position separates them from content exactly. + """ + diff_text = """diff --git a/src/counter.cpp b/src/counter.cpp +index 1111111..2222222 100644 +--- a/src/counter.cpp ++++ b/src/counter.cpp +@@ -1,2 +1,4 @@ + int main() { ++++counter_increment; ++ normal_added_line(); +""" + + # The added source line is `++counter_increment;`, so the diff renders it + # as `+++counter_increment;` -- indistinguishable from a file header by + # prefix alone. + assert changed_lines.extract_added_lines(diff_text) == ( + "++counter_increment;\n normal_added_line();\n" + ) + + +def test_file_headers_are_still_skipped_across_several_files() -> None: + """The hunk-position rule must not start admitting real `+++ b/path` headers.""" + diff_text = """diff --git a/a.md b/a.md +--- a/a.md ++++ b/a.md +@@ -1 +1,2 @@ ++first heading +diff --git a/b.md b/b.md +--- a/b.md ++++ b/b.md +@@ -1 +1,2 @@ ++second heading +""" + + assert changed_lines.extract_added_lines(diff_text) == "first heading\nsecond heading\n" + + +def test_fallback_emits_a_workflow_annotation_only_under_github_actions( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The `::warning::` form is what makes an over-reporting run visible. + + On stderr alone the notice is buried in the step log, so a run that fell + back is indistinguishable from a correctly scoped one at the point someone + reads the check result. The annotation is gated on GITHUB_ACTIONS so a local + run does not print workflow-command syntax at a developer. + """ + repo = tmp_path / "unrelated" + repo.mkdir() + git(repo, "init", "--quiet", "--initial-branch=main") + git(repo, "config", "user.email", "ci@example.invalid") + git(repo, "config", "user.name", "CI") + (repo / "notes.md").write_text("Only history.\n", encoding="utf-8") + git(repo, "add", "notes.md") + git(repo, "commit", "--quiet", "--message", "only commit") + + monkeypatch.setenv("GITHUB_ACTIONS", "true") + changed_lines.resolve_merge_base(repo, "refs/heads/no-such-branch") + captured = capsys.readouterr() + assert "::warning::changed_lines: cannot resolve merge base" in captured.err + assert captured.out == "" + + monkeypatch.setenv("GITHUB_ACTIONS", "false") + changed_lines.resolve_merge_base(repo, "refs/heads/no-such-branch") + captured = capsys.readouterr() + assert "cannot resolve merge base" in captured.err + assert "::warning::" not in captured.err + + def test_extension_pathspecs_are_normalized_for_git_diff() -> None: extensions = changed_lines.normalize_extensions("md,.txt, py,,")