From 1c1880090af99da9d64d11249a9869ef732e2170 Mon Sep 17 00:00:00 2001 From: Imran Siddique Date: Thu, 6 Aug 2026 09:01:07 -0700 Subject: [PATCH 1/3] fix(ci): scope spell-check to the PR's own diff, not a stale base ref `git fetch origin ` only writes FETCH_HEAD unless the configured refspec covers the branch. actions/checkout narrows remote.origin.fetch to the ref it checked out, so on a pull request `refs/remotes/origin/` keeps whatever value it had at checkout time. `--base origin/` then resolves a merge base against that stale ref. It succeeds, so no warning fires, but it points far enough back that every commit landing on the base since is reported as added. PR #3567 changes two TypeScript files totalling 83 added lines and was spell-checked against several hundred lines it never touched, failing on words absent from its diff (AEDT, anthonyonazure, Clendenen, langgenius, ringbreachdetector). 22 of 68 open PRs currently have a red spell-check. - Fetch the base with an explicit refspec so the remote-tracking ref is actually updated. - Emit the merge-base fallback as a ::warning:: annotation as well as on stderr. On stderr alone an over-reporting run is indistinguishable from a correctly scoped one at the point where the check result is read, which is how this went unnoticed while failing unrelated PRs. - Add the two words genuinely introduced by open PRs: `linkbase` (#3620) and `unpermitted` (#3200). The rest of the currently-flagged words are artifacts of the over-report and go away with the scoping fix. Refs #3514. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Imran Siddique --- .cspell-repo-terms.txt | 4 ++++ .github/workflows/spell-check.yml | 12 +++++++++++- scripts/ci/changed_lines.py | 14 ++++++++++---- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/.cspell-repo-terms.txt b/.cspell-repo-terms.txt index f3be486fa..f1ddb09f3 100644 --- a/.cspell-repo-terms.txt +++ b/.cspell-repo-terms.txt @@ -1009,6 +1009,10 @@ hostnames dataprotection # dorny/paths-filter — GitHub Actions dependency used in policy-validation.yml dorny +# 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..983da33a5 100644 --- a/.github/workflows/spell-check.yml +++ b/.github/workflows/spell-check.yml @@ -42,10 +42,20 @@ 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 because `git fetch origin ` only writes + # FETCH_HEAD unless the configured refspec happens to cover the branch. + # actions/checkout narrows remote.origin.fetch to the ref it checked out, + # so on a PR the wildcard is absent and `refs/remotes/origin/` keeps + # whatever value it had at checkout time. `--base origin/` below then + # resolves a merge base against a stale ref, which succeeds (no warning) + # while pointing far enough back that every commit landing on the base + # since is reported as added. That is how a two-file PR came to be + # spell-checked against several hundred lines it never touched. - 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..1104084f2 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 @@ -75,10 +76,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 From a37ceb8b1c9f84a4b29698add173c52e2e46c5a7 Mon Sep 17 00:00:00 2001 From: Imran Siddique Date: Wed, 12 Aug 2026 10:40:11 -0700 Subject: [PATCH 2/3] fix(ci): correct the scoping rationale and stop dropping ++ content lines Rewrites the spell-check.yml comment to present the explicit refspec as hardening rather than as the fix for an observed over-scan. The stale-ref causal claim did not hold: #3530 had already corrected the scoping, and runs after it scope correctly. Also fixes a real defect in extract_added_lines. The `+++ b/path` header was skipped by prefix, which silently drops an added line whose own content starts with `++` (rendered `+++...` in the diff). Skipping by hunk position separates the two exactly. Adds a regression test that fails without the fix, plus coverage for the GITHUB_ACTIONS annotation branch. Signed-off-by: Imran Siddique --- .github/workflows/spell-check.yml | 20 ++++---- scripts/ci/changed_lines.py | 20 +++++++- tests/ci/test_changed_lines.py | 78 +++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 11 deletions(-) diff --git a/.github/workflows/spell-check.yml b/.github/workflows/spell-check.yml index 983da33a5..c713e802e 100644 --- a/.github/workflows/spell-check.yml +++ b/.github/workflows/spell-check.yml @@ -42,15 +42,17 @@ 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 because `git fetch origin ` only writes - # FETCH_HEAD unless the configured refspec happens to cover the branch. - # actions/checkout narrows remote.origin.fetch to the ref it checked out, - # so on a PR the wildcard is absent and `refs/remotes/origin/` keeps - # whatever value it had at checkout time. `--base origin/` below then - # resolves a merge base against a stale ref, which succeeds (no warning) - # while pointing far enough back that every commit landing on the base - # since is reported as added. That is how a two-file PR came to be - # spell-checked against several hundred lines it never touched. + # 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 diff --git a/scripts/ci/changed_lines.py b/scripts/ci/changed_lines.py index 1104084f2..47ec7d05c 100644 --- a/scripts/ci/changed_lines.py +++ b/scripts/ci/changed_lines.py @@ -32,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:]) diff --git a/tests/ci/test_changed_lines.py b/tests/ci/test_changed_lines.py index 12be181c2..344f85433 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_with_a_typoo; ++ normal_added_line(); +""" + + # The added source line is `++counter_with_a_typoo;`, so the diff renders it + # as `+++counter_with_a_typoo;` -- indistinguishable from a file header by + # prefix alone. + assert changed_lines.extract_added_lines(diff_text) == ( + "++counter_with_a_typoo;\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 tokenn +diff --git a/b.md b/b.md +--- a/b.md ++++ b/b.md +@@ -1 +1,2 @@ ++second tokenn +""" + + assert changed_lines.extract_added_lines(diff_text) == "first tokenn\nsecond tokenn\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,,") From 273d8ea1af715f42bca082e020d6b8ffb4eb4a3b Mon Sep 17 00:00:00 2001 From: Imran Siddique Date: Wed, 12 Aug 2026 10:43:11 -0700 Subject: [PATCH 3/3] test(ci): drop deliberate misspellings from new fixtures, allow refspec The new tests exercise extract_added_lines, which only extracts, so the fixtures never needed misspelled words. Using them meant the added lines reintroduced typoo and tokenn into the spell-check scan, which the job correctly flagged. Adds refspec to the repo terms: a genuine git term now used in the spell-check.yml comment. Signed-off-by: Imran Siddique --- .cspell-repo-terms.txt | 2 ++ tests/ci/test_changed_lines.py | 14 +++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/.cspell-repo-terms.txt b/.cspell-repo-terms.txt index f1ddb09f3..ec2923035 100644 --- a/.cspell-repo-terms.txt +++ b/.cspell-repo-terms.txt @@ -1009,6 +1009,8 @@ 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) diff --git a/tests/ci/test_changed_lines.py b/tests/ci/test_changed_lines.py index 344f85433..7d5f35fb2 100644 --- a/tests/ci/test_changed_lines.py +++ b/tests/ci/test_changed_lines.py @@ -67,15 +67,15 @@ def test_added_line_whose_content_starts_with_plus_plus_is_not_dropped() -> None +++ b/src/counter.cpp @@ -1,2 +1,4 @@ int main() { -+++counter_with_a_typoo; ++++counter_increment; + normal_added_line(); """ - # The added source line is `++counter_with_a_typoo;`, so the diff renders it - # as `+++counter_with_a_typoo;` -- indistinguishable from a file header by + # 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_with_a_typoo;\n normal_added_line();\n" + "++counter_increment;\n normal_added_line();\n" ) @@ -85,15 +85,15 @@ def test_file_headers_are_still_skipped_across_several_files() -> None: --- a/a.md +++ b/a.md @@ -1 +1,2 @@ -+first tokenn ++first heading diff --git a/b.md b/b.md --- a/b.md +++ b/b.md @@ -1 +1,2 @@ -+second tokenn ++second heading """ - assert changed_lines.extract_added_lines(diff_text) == "first tokenn\nsecond tokenn\n" + assert changed_lines.extract_added_lines(diff_text) == "first heading\nsecond heading\n" def test_fallback_emits_a_workflow_annotation_only_under_github_actions(